vimPluginsUpdater: rewrite to use nupd #380691
Open
+60,629
−21,986
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
github-actions
bot
added
6.topic: vim
8.has: documentation
nix-owners
bot
requested review from
mrcjkb,
rvolosatovs,
figsoda,
khaneliman and
GaetanLepage
|
Now update of all plugins is faster than my git status! Also, please don't merge any new plugins until this is merged |
PerchunPak
force-pushed
the
vim-updater-nupd
branch
from
51984bc to
1287ee2
Compare
nix-owners
bot
requested review from
hsjobeki,
GetPsyched and
fricklerhandwerk
PerchunPak
force-pushed
the
vim-updater-nupd
branch
from
1287ee2 to
3dffe72
Compare
|
Weird, eval works for me |
khaneliman
reviewed
Feb 9, 2025
|
|
||
| ``` | ||
| nix-shell -p vimPluginsUpdater --run vim-plugins-updater -i vim-plugin-names -o generated.nix --no-commit | ||
| nix-shell -p vimPluginsUpdater --run 'vim-plugins-updater update "https://github.com/folke/lazy.nvim"' |
There was a problem hiding this comment.
Does this no longer support by plugin name? Seems more appropriate than having to enter an entire url everytime
ie:
vim-plugins-updater update lazy-nvim
There was a problem hiding this comment.
It uses the same system as add command. I plan to remove https://github.com/ that is required for every plugin in the near future
There was a problem hiding this comment.
Hmm... alright :S Not a fun regression, but we rarely do individual updates anyways, I suppose.
Do you plan to leave a single commit for this ? |
khaneliman
reviewed
Feb 9, 2025
khaneliman
reviewed
Feb 9, 2025
Comment on lines
110
to
111
| _default_input_file: Path = field(init=False, default=ROOT / "input.csv") | ||
| _default_output_file: Path = field(init=False, default=ROOT / "output.json") |
There was a problem hiding this comment.
Can we use better names than input/output
There was a problem hiding this comment.
Changed to generated-input.csv and generated.json
This comment was marked as outdated.
This comment was marked as outdated.
PerchunPak
force-pushed
the
vim-updater-nupd
branch
5 times, most recently
from
fe7934a to
f7d2070
Compare
nix-owners
bot
requested review from
MarcWeber,
melnary,
millerjason and
jorsn
|
No new regressions so far
|
|
One thing that really bothered us about the previous updater was inability to pin plugins to releases. Does this allow pinning packages to use release tags? (Completely understand if that was out of scope on initial rewrite, just hoping) |
This was out of scope, but it is simple enought to implement later |
|
|
What are all the failures from ? First one I checked doesn't fail on master. I'm seeing hash mismatches on a couple so far in a nixpkgs review. |
|
There was a problem hiding this comment.
Not reformatting the entire docs will make this PR reviewable.
There was a problem hiding this comment.
I moved formatting changes to a separate commit, hope this will make it reviewable
| """ | ||
| ) | ||
| @define | ||
| class MyImpl(ABCBase[MyEntry, MyEntryInfo]): |
There was a problem hiding this comment.
Suggested change
| class MyImpl(ABCBase[MyEntry, MyEntryInfo]): | |
| class Impl(ABCBase[Entry, EntryInfo]): |
Otherwise the code reads like it got straight copied from the example in the docs.
There was a problem hiding this comment.
Changed to VimImpl, VimEntry, VimEntryInfo (the same prefix that was used before)
13 tasks
PerchunPak
force-pushed
the
vim-updater-nupd
branch
2 times, most recently
from
d866356 to
2a806d4
Compare
There was a problem hiding this comment.
I am quite unfamiliar with the paradigm you chose for the update.py python script.
I would need to read more to really understand how it works.
If another reviewer is more technically able than me on this technology and wishes to review this PR, please go on :)
| "# GENERATED by ./pkgs/applications/editors/vim/plugins/update.py. Do not edit!" | ||
| ) | ||
| @classmethod | ||
| def parse(cls, inp: str) -> t.Self: |
There was a problem hiding this comment.
What is inp suppose to stand for?
Maybe I am missing something obvious.
In any case, I feel like a more explicit variable name would be better.
There was a problem hiding this comment.
This function parses a GitHub URL to a GitHub repo object
It gets called e.g. on values from generated-input.csv or if user provided some plugins to update
The context of this function is very informative, if you look directly at file, not at diff
class GHRepoInfo:
@classmethod # classmethods usually initialize a class in which they live
def parse(cls, inp: str) -> t.Self:
# | | |
# | | └- returns an instance of self
# | └- an input == anything that will be parsed
# └- parse something, e.g. gh URL to the objectThere was a problem hiding this comment.
Thanks for the clarification. Indeed it is very fine.
My only comment was about the name of the inp variable. I was not able to tell that it means "input".
Please, just rename it to input.
The rest is perfectly clear and straightforward.
There was a problem hiding this comment.
Would be helpful to include docstrings throughout.
There was a problem hiding this comment.
Would be helpful to include docstrings throughout.
I am not sure how to write it to not be
def do_x_thing():
"""Does X thing."""
Yeah, I will need to write docs for nupd eventually |
On masterGenerated using Command:
|
On masterGenerated using Command:
|
The upstream was deprecated for two years and then removed. Consider using Lualine
PerchunPak
force-pushed
the
vim-updater-nupd
branch
from
2a806d4 to
b285086
Compare
On this PRGenerated using Command:
|
PerchunPak
force-pushed
the
vim-updater-nupd
branch
2 times, most recently
from
3998f46 to
1b64abc
Compare
The new updater doesn't do this automatically, but it is still has to be done after nvim-treesitter update
PerchunPak
force-pushed
the
vim-updater-nupd
branch
from
1b64abc to
d2622ad
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Things done
nix.conf? (See Nix manual)sandbox = relaxedsandbox = truenix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/)CC @NixOS/neovim
Add a 👍 reaction to pull requests you find important.