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

doc: give neovim its own neovim.section.md #373090

Merged
merged 1 commit into from
Jan 12, 2025

Conversation

teto
Copy link
Member

@teto teto commented Jan 12, 2025

  • doc/neovim: move neovim to its own section
  • wip: fixing the redirects.json

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 25.05 Release Notes (or backporting 24.11 and 25.05 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

Add a 👍 reaction to pull requests you find important.

Copy link
Member

@mrcjkb mrcjkb left a comment

Choose a reason for hiding this comment

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

Not strictly blocking for this PR (as it just moves stuff around), but I've made some suggestions for the wording :)

[approval pending the fix of the manual build failure, of course]

doc/languages-frameworks/neovim.section.md Outdated Show resolved Hide resolved
### Testing Neovim plugins {#testing-neovim-plugins}

#### neovimRequireCheck {#testing-neovim-plugins-neovim-require-check}
`neovimRequireCheck` is a simple test which checks if Neovim can requires lua modules without errors. This is often enough to catch missing dependencies.
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
`neovimRequireCheck` is a simple test which checks if Neovim can requires lua modules without errors. This is often enough to catch missing dependencies.
`neovimRequireCheck` is a simple test which checks if Neovim can load a plugin's lua modules without errors. This is often enough to catch missing dependencies.

Copy link
Member Author

Choose a reason for hiding this comment

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

I prefer the "require" mention that makes it clearer for the initiated (which this subsection is for).

doc/languages-frameworks/neovim.section.md Outdated Show resolved Hide resolved
When `nvimRequireCheck` is not specified, we will search the plugin's directory for lua modules to attempt loading. This quick smoke test can catch obvious dependency errors that might be missed.
The check hook will fail the build if any failures are detected to encourage inspecting the logs to identify potential issues.

If you would like to only check a specific module, this can be manually added through plugin definition overrides in the [overrides.nix](https://github.com/NixOS/nixpkgs/blob/master/pkgs/applications/editors/vim/plugins/overrides.nix).
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
If you would like to only check a specific module, this can be manually added through plugin definition overrides in the [overrides.nix](https://github.com/NixOS/nixpkgs/blob/master/pkgs/applications/editors/vim/plugins/overrides.nix).
If you would like to only check a specific module, it can be added manually via the plugin definition overrides in [overrides.nix](https://github.com/NixOS/nixpkgs/blob/master/pkgs/applications/editors/vim/plugins/overrides.nix).

Copy link
Member Author

@teto teto Jan 12, 2025

Choose a reason for hiding this comment

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

I'll rewrite it to the even shorter .., add it to the plugin definition in [overrides.nix]

```

In rare cases, we might not want to actually test loading lua modules for a plugin. In those cases, we can disable `neovimRequireCheck` with `doCheck = false;`.

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
Doing so will also prevent the plugin's test suite from running (if present).

Copy link
Member Author

Choose a reason for hiding this comment

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

@khaneliman is there any instance of us disabling doCheck just because of neovimRequireCheck ? If not let's delete this part.

Copy link
Member Author

Choose a reason for hiding this comment

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

also there is a mix of neovimRequireCheck and nvimRequireCheck. Maybe you want to talk about neovimRequireCheckHook but it can be fixed in a separate PR.

Copy link
Contributor

@khaneliman khaneliman Jan 12, 2025

Choose a reason for hiding this comment

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

Most other language/framework sections contain some mention about the usage of doCheck = false; to skip the check phase. This is just an explanation of why you might want to use it. So I'd rather keep it but maybe update the example with some more accurate.

  supermaven-nvim = super.supermaven-nvim.overrideAttrs {
    # TODO: handle supermaven binary
    doCheck = false;
  };

I wanted to add instructions on how to configure neovim via the new
wrapper but it was difficult mixing this with both the vim and old
wrapper.
Neovim differs enough from vim to warrant its own section IMO:
1. its wrapper is different (old wrapper close to vim's syntax, new one
   not so much)
2. treesitter is unique to neovim
3. the section about neovim plugins is unique to neovim as well. Not
   only that but it needs to expanded.

At some point the doc unique to vim is going to exceed vim's.
We can refer to vim's section to avoid duplication where it makes sense.
@teto teto force-pushed the teto/neovim-document-new-wrapper branch from e9e1f74 to ec51c28 Compare January 12, 2025 21:34
@teto teto marked this pull request as ready for review January 12, 2025 21:34
@teto teto merged commit 3f2423e into NixOS:master Jan 12, 2025
26 checks passed
@teto teto deleted the teto/neovim-document-new-wrapper branch January 12, 2025 22:33
"index.html#vim-plugin-specificities"
"neovim-plugin-specificities": [
"index.html#neovim-plugin-specificities",
"neovim-plugin-specificities#vim-plugin-specificities"
Copy link
Contributor

@fricklerhandwerk fricklerhandwerk Jan 13, 2025

Choose a reason for hiding this comment

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

This should probably have been index.html#vim-plugin-specificities?

But great to see that @GetPsyched's work is useful to avoid documentation falling apart 😄

Copy link
Member Author

Choose a reason for hiding this comment

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

maybe ? Because I had seen the PR about the redirection and all I had an idea of what was going on, otherwise I think the feedback would have surprised me more. I understand the idea behind the redirects but as a maintainer it's one additional hurdle to contribute to the doc and the UX frustrated me quite a bit. I spent more time trying to work out the redirections rather than the doc changes. Note that I contribute to the doc reluctantly as a last resort so I am already grumpy by then ^^'
First I think the errors should give a bit of context otherwise these errors popup without explanation.
I dont know how hard that is but if we could use redirects.jsonc instead of redirects.json, one could document the format a bit at the top of the file and document the redirects later (it might not matter much though).
the redirects executable differentiate between rename and moved content but it seemed articial. I believe this are no checks on the input arguments ? several times I ended up with wrong anchors or duplicates.
Is there any other possible value for path than "index.html" ? else we could default to that.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for the feedback! And sorry that the experience was frustrating. Good point about input validation! The format is documented in the source code for the tool dealing with it, but it sounds reasonable to indeed document it in place. rename and move are different because the ultimate goal is to be able to change the page to where content is rendered, which is not implemented yet.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: 👀 To Review
Development

Successfully merging this pull request may close these issues.

4 participants