Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add CI via Github Actions #553

Closed
wants to merge 1 commit into from
Closed

Add CI via Github Actions #553

wants to merge 1 commit into from

Conversation

hyiltiz
Copy link

@hyiltiz hyiltiz commented Dec 22, 2023

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.

@feoh
Copy link
Collaborator

feoh commented Dec 27, 2023

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
Copy link
Collaborator

feoh commented Jan 8, 2024

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!

@hyiltiz
Copy link
Author

hyiltiz commented Jan 9, 2024

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?

@nPHYN1T3
Copy link
Contributor

nPHYN1T3 commented Jan 9, 2024

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

@dam9000
Copy link
Contributor

dam9000 commented Jan 12, 2024

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: #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: #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.

@feoh
Copy link
Collaborator

feoh commented Jan 12, 2024

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 feoh closed this Jan 12, 2024
@hyiltiz
Copy link
Author

hyiltiz commented Jan 12, 2024

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.

@feoh
Copy link
Collaborator

feoh commented Jan 12, 2024

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants