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

plugins/guess-indent: init #1918

Merged
merged 2 commits into from
Jul 25, 2024
Merged

Conversation

GGORG0
Copy link
Contributor

@GGORG0 GGORG0 commented Jul 24, 2024

Copy link
Member

@GaetanLepage GaetanLepage left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks in a very good state already.

Comment on lines 7 to 9
}:
helpers.neovim-plugin.mkNeovimPlugin config {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
}:
helpers.neovim-plugin.mkNeovimPlugin config {
}:
with lib;
helpers.neovim-plugin.mkNeovimPlugin config {

Even though this is not recommended in nixpkgs, we adopted this convention across the nixvim codebase.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we reconsider this? Perhaps we could inherit (lib) X Y Z instead?

At least in new files - I'm not proposing we go around changing this in existing code for the sake of it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 on not using with

nixpkgs has a tendency on getting rid of it, at least partially (NixOS/nixpkgs#292468) and best practices also suggests that inherit should be preferred.

@GGORG0
Copy link
Contributor Author

GGORG0 commented Jul 24, 2024

Thanks for the blazingly fast review! Also, I didn't run the testing suite (because it is just too heavy for my machine), so I didn't notice the maintainers error

@MattSturgeon
Copy link
Member

MattSturgeon commented Jul 24, 2024

I didn't run the testing suite (because it is just too heavy for my machine)

That's reasonable, however note that in the devshell (nix develop) we have a (slightly) lighter tests command that only runs the tests in /tests/test-sources rather than all checks. It's also possible to select just the specific tests you want to run.

@MattSturgeon
Copy link
Member

Out of curiosity: does this plugin have the same goal as tpope's slueth? How does it differ?

@GGORG0
Copy link
Contributor Author

GGORG0 commented Jul 24, 2024

Out of curiosity: does this plugin have the same goal as tpope's slueth? How does it differ?

Yes, it does. However it's written for nvim specifically. Here you can see the comparison: https://github.com/NMAC427/guess-indent.nvim/tree/6cd61f7a600bb756e558627cd2e740302c58e32d?tab=readme-ov-file#alternatives

@GGORG0
Copy link
Contributor Author

GGORG0 commented Jul 25, 2024

Do the CI checks normally take 20+ hours? 😅 That's so dumb that there isn't any caching or selective testing involved. The reviews and bugfixing were done in <30 minutes, and the tests are literally still running...

@MattSturgeon
Copy link
Member

Do the CI checks normally take 20+ hours? 😅 That's so dumb that there isn't any caching or selective testing involved. The reviews and bugfixing were done in <30 minutes, and the tests are literally still running...

The tests are actually finished (all passed) but something has gone wrong reporting some of them to GitHub (because there's over 1000 and we have hit a rate limit)

@GGORG0
Copy link
Contributor Author

GGORG0 commented Jul 25, 2024

The tests are actually finished (all passed)

Still, they have finished just 4 hours ago. That's 16 hours.

but something has gone wrong reporting some of them to GitHub (because there's over 1000 and we have hit a rate limit)

Yeah, I'm not surprised - maybe they should just report as 1 huge test, or a small number of categorized tests.

By the way, is there anything else to do with this PR or is it ready to merge (and GitHub is preventing that because of the unfinished tests)?

@MattSturgeon
Copy link
Member

MattSturgeon commented Jul 25, 2024

By the way, is there anything else to do with this PR or is it ready to merge (and GitHub is preventing that because of the unfinished tests)?

Could you rebase on main and squash to two commits (one adding you to maintainers, and another adding the plugin)?

Only the nix-eval test needs to be reported as passed for mergify to be happy, but we can merge manually without it.

The main thing is having at least one approving review.

I'll try and review properly shortly!

Yeah, I'm not surprised - maybe they should just report as 1 huge test, or a small number of categorized tests.

We had one huge test until very recently, which buildbot couldn't handle as the memory requirements ended up being extreme.

We'll probably end up with a compromise where we have maybe 20 grouped tests, but we're still experimenting with finding a good balance.

There's also effort upstream in buildbot to improve how many tests get reported.

Copy link
Member

@MattSturgeon MattSturgeon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, once squashed and rebased (see my last comment). Thanks for your efforts!

@GGORG0
Copy link
Contributor Author

GGORG0 commented Jul 25, 2024

Could you rebase on main and squash to two commits (one adding you to maintainers, and another adding the plugin)?

I think I did what I was supposed to... I had to learn about rebasing, because this is my first serious PR to a big project

@MattSturgeon

This comment was marked as outdated.

@MattSturgeon
Copy link
Member

Could you rebase on main and squash to two commits (one adding you to maintainers, and another adding the plugin)?

I think I did what I was supposed to... I had to learn about rebasing, because this is my first serious PR to a big project

Looks perfect, thanks!

This comment was marked as outdated.

@MattSturgeon MattSturgeon merged commit 30ab203 into nix-community:main Jul 25, 2024
659 of 1144 checks passed
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.

None yet

4 participants