Skip to content

vimPlugins.render-markdown-nvim: add nvim-treesitter dependency#343557

Merged
Aleksanaa merged 2 commits intoNixOS:masterfrom
GaetanLepage:render-markdown
Sep 24, 2024
Merged

vimPlugins.render-markdown-nvim: add nvim-treesitter dependency#343557
Aleksanaa merged 2 commits intoNixOS:masterfrom
GaetanLepage:render-markdown

Conversation

@GaetanLepage
Copy link
Contributor

@GaetanLepage GaetanLepage commented Sep 21, 2024

Description of changes

From their README
it seems that treesitter is required.
The rest look like optional dependencies.

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/)
  • 24.11 Release Notes (or backporting 23.11 and 24.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.

@github-actions github-actions bot added the 6.topic: vim Advanced text editor label Sep 21, 2024
@GaetanLepage
Copy link
Contributor Author

Result of nixpkgs-review pr 343557 run on x86_64-linux 1

2 packages built:
  • vimPlugins.render-markdown-nvim
  • vimPluginsUpdater

@ofborg ofborg bot added 8.has: clean-up This PR removes packages or removes other cruft 8.has: package (new) This PR adds a new package 10.rebuild-darwin: 1-10 This PR causes between 1 and 10 packages to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. labels Sep 21, 2024
@GaetanLepage GaetanLepage requested a review from teto September 21, 2024 21:15
@teto
Copy link
Member

teto commented Sep 22, 2024

it should not be too hard to test the plugin. add the dependencies into RTP (since nixpkgs infra doesn't do that yet) , open a markdown and see if if neovim errors out. like
nvim -c"set rtp+=${vimPlugins.nvim-treesitter}" -c "${render-markdown-nvim}" "PreviewMarkdown" -es +quitall!

Some tests are visible for reference at
#343769

Copy link
Member

Choose a reason for hiding this comment

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

nvimRequireCheck = "render-markdown"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure ! Done.

Copy link
Member

@PerchunPak PerchunPak left a comment

Choose a reason for hiding this comment

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

LGTM

@Aleksanaa Aleksanaa merged commit 804a180 into NixOS:master Sep 24, 2024
@GaetanLepage GaetanLepage deleted the render-markdown branch September 24, 2024 05:21
@GaetanLepage
Copy link
Contributor Author

it should not be too hard to test the plugin. add the dependencies into RTP (since nixpkgs infra doesn't do that yet) , open a markdown and see if if neovim errors out.

This didn't make it to the PR in the end.
Do you have a plugin examples where those tests are used already ?

@mrcjkb
Copy link
Member

mrcjkb commented Sep 24, 2024

This is one of those cases where the plugin doesn't actually depend on nvim-treesitter, but it requires one or multiple parsers to be installed.

In such cases, adding nvim-treesitter as a dependency won't make the plugin work out of the box. Users will still have to specify nvim-treesitter.withPlugins ...
There's no need to add nvim-treesitter as a dependency if the plugin doesn't actually try to require one of its (legacy) modules.

@GaetanLepage
Copy link
Contributor Author

This is one of those cases where the plugin doesn't actually depend on nvim-treesitter, but it requires one or multiple parsers to be installed.

In such cases, adding nvim-treesitter as a dependency won't make the plugin work out of the box. Users will still have to specify nvim-treesitter.withPlugins ... There's no need to add nvim-treesitter as a dependency if the plugin doesn't actually try to require one of its (legacy) modules.

Right, I forgot about this. I will revert it to prevent such noise from remaining in the code base.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

6.topic: vim Advanced text editor 8.has: clean-up This PR removes packages or removes other cruft 8.has: package (new) This PR adds a new package 10.rebuild-darwin: 1-10 This PR causes between 1 and 10 packages to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants