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

vimPlugins.leetcode-nvim: init at 2024-06-27 #340378

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

nvimtor
Copy link
Contributor

@nvimtor nvimtor commented Sep 7, 2024

Description of changes

Adds the plugin leetcode.nvim.

Includes hard dependencies telescope-nvim and nui-nvim.

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.

@linsui
Copy link
Contributor

linsui commented Sep 10, 2024

Close #274061

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! The order doesn't matter as it is broken anyway

Fixes #274061

CC @GaetanLepage

@nvimtor nvimtor force-pushed the leetcode-nvim branch 2 times, most recently from c3a3236 to cb98609 Compare September 18, 2024 09:25
@nvimtor
Copy link
Contributor Author

nvimtor commented Sep 18, 2024

Also updated the SHA as it was incorrect; apologies. I double checked it with nurl and it should be the correct hash now.

Copy link
Contributor

@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.

I have made minor cosmetic changes.

@GaetanLepage
Copy link
Contributor

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

1 package failed to build:
  • vimPlugins.leetcode-nvim
1 package built:
  • vimPluginsUpdater

@GaetanLepage
Copy link
Contributor

Looks like dependencies are not transitive: telescope-nvim has plenary-nvim as a dependency, but leetcode-nvim won't see it (despite it having telescope-nvim as a dependency).

cc @teto (I have been pinging you a lot lately, sorry for that)

@GaetanLepage
Copy link
Contributor

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

1 package failed to build:
  • vimPlugins.leetcode-nvim
1 package built:
  • vimPluginsUpdater

1 similar comment
@GaetanLepage
Copy link
Contributor

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

1 package failed to build:
  • vimPlugins.leetcode-nvim
1 package built:
  • vimPluginsUpdater

@GaetanLepage
Copy link
Contributor

Check whether the following module can be imported: leetcode
Error detected while processing pre-vimrc command line:
E518: Unknown option: /nix/store/9711k560h24xp38drsrdzjawfm3v6i2p-vimplugin-lua5.1-telescope.nvim-scm-1-unstable-2024-09-11
Entering Ex mode.  Type "visual" to go to Normal mode.

Copy link
Contributor

@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.

We need to fix the require check.

@PerchunPak
Copy link
Member

PerchunPak commented Sep 23, 2024

Blocked by #342240 (tracker)

@teto
Copy link
Member

teto commented Sep 23, 2024

Looks like dependencies are not transitive: telescope-nvim has plenary-nvim as a dependency, but leetcode-nvim won't see it (despite it having telescope-nvim as a dependency).

ha indeed. The neovimRequireCheckHook just adds the listed dependencies to RTP. a quickfix could be to add vimUtils.transitiveClosure dependencies to rtp instead. It still wont work for plugins that pull python provider/extra programs dependencies etc but could work for 90% plugins.
Ideally we would use the same utilities to generate the wrapped neovim and the tests. We are getting there :)

@PerchunPak
Copy link
Member

ha indeed. The neovimRequireCheckHook just adds the listed dependencies to RTP. a quickfix could be to add vimUtils.transitiveClosure dependencies to rtp instead. It still wont work for plugins that pull python provider/extra programs dependencies etc but could work for 90% plugins. Ideally we would use the same utilities to generate the wrapped neovim and the tests. We are getting there :)

#342240 fixes it, no?

@teto
Copy link
Member

teto commented Sep 23, 2024

#342240 fixes it, no?

I dont think so, it will just expand the derivation dependencies without the transitives ones, as mentioned by gaetan.

@PerchunPak
Copy link
Member

PerchunPak commented Oct 3, 2024

git cherry-pick b858149 dc2ec23 && nix-build -A vimPlugins.leetcode-nvim builds succesful, so... it is fixed by #342240

@GaetanLepage
Copy link
Contributor

git cherry-pick b858149 dc2ec23 && nix-build -A vimPlugins.leetcode-nvim builds succesful, so... it is fixed by #342240

@nvimtor can you go ahead and rebase this then please ?

@PerchunPak
Copy link
Member

PerchunPak commented Oct 4, 2024

nvimtor can you go ahead and rebase this then please ?

No, because b858149 is #342240, which is currently in staging-next and not in master (https://nixpk.gs/pr-tracker.html?pr=342240)

EDIT: see also #343421

@GaetanLepage
Copy link
Contributor

No, because b858149 is #342240, which is currently in staging-next and not in master (https://nixpk.gs/pr-tracker.html?pr=342240)

Oh right, I got tricked by the "merged" status.

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

Successfully merging this pull request may close these issues.

6 participants