kickstart.nvim icon indicating copy to clipboard operation
kickstart.nvim copied to clipboard

Add CI via Github Actions

Open hyiltiz opened this issue 2 years ago • 1 comments

Adding CI support via Github Actions. Compared to the previous PR #550, this one:

  • adds .luacheckrc and runs linting on the init.lua and lua/
  • combines the previous style check into a single CI test file
  • adds a build CI badge on README.md top for nvim-lua/kickstart.nvim repo
  • squashes all commits into one
  • reformats a long line to be compliant with style guides (so no more errors from stylua and luacheck
  • throws 0 errors or warnings for nvim-lua/kickstart.nvim:master branch as of now.

hyiltiz avatar Dec 22 '23 03:12 hyiltiz

I've been thinking about this, and while I appreciate the desire to improve code quality, I'm concerned about what the new committer experience would be like.

As a result I'm hesitant to merge this and would love it if folks would chime in one way or the other.

feoh avatar Dec 27 '23 00:12 feoh

Hi. I don't want you to think I'm ignoring this, but as I said I'm not sure whether or not this is desirable, so if you feel strongly about this contribution please get others in the community to approve and if you can, I'll merge it.

If you don't feel strongly, please go ahead and close this or I will in a week or so.

In any case thank you for your contribution!

feoh avatar Jan 08 '24 21:01 feoh

Pulling in some of the contributors for an opinion: @dam9000 @dilshod @juangiordana @tjdevries @nPHYN1T3 (please pitch in if you happen to have some time, and sorry about the ping)

How would you like this PR to change before it would be ready to merge?

hyiltiz avatar Jan 09 '24 05:01 hyiltiz

No need to apologize about the ping, I've got time but I'd wager I'm not qualified to chime in.

nPHYN1T3 avatar Jan 09 '24 06:01 nPHYN1T3

In my view this is perhaps trying to do too much for such a small project as the kickstart.nvim is. From the past few months of following this project I'm not seeing that the contributors would introduce such breaking changes that would be caught by this CI script. As for external breaking changes, they also have to be very rare, the only one I've seen so far would be this: https://github.com/nvim-lua/kickstart.nvim/issues/537 and this CI script would not help to catch it because a fresh install would work fine (what this CI script does) but a pre-existing install doing a lazy plugin update would fail.

If this is decided to be merged it still needs to address at least two issues, one is merge conflicts and the other is the non-automatic workflow approval which is dictated by the project github settings, this can be worked around with, see this issue for details: https://github.com/nvim-lua/kickstart.nvim/issues/570

All in all I'd say it's a nice piece of work, but I don't know if the project needs it. Whatever is decided is fine with me, I'm not a maintainer so it's not my call. If it's not merged and you believe it offers some benefit you could still run the periodic schedule part on your own fork and if it detects an error you would be able to report it, and perhaps make a case then for it's usefulness.

dam9000 avatar Jan 12 '24 09:01 dam9000

That's my feeling too. Thanks very much for your contribution. It's a nice piece of work, I just don't think it's appropriate for this project in particular.

feoh avatar Jan 12 '24 16:01 feoh

Thanks all for the feedback! I'll track all upstream changes in my fork and run this to evaluate its value. I'll report back if I find anything.

hyiltiz avatar Jan 12 '24 16:01 hyiltiz

That would be awesome thank you very much! Pull requests gleefully accepted!

feoh avatar Jan 12 '24 16:01 feoh