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

pluginupdate.py: fix bugs and add improvements; vimPlugins: sort properly #353786

Merged
merged 3 commits into from
Nov 9, 2024

Conversation

PerchunPak
Copy link
Member

I fixed many hidden bugs and made some small improvements. The main reason this was separated from #336137 is to fix the sorting issue.

Before this commit, sorting for Vim plugins was broken and worked by what was fetched first. This is because the sorting was done by empty strings (the default value in CSV is not None but an empty string). This PR fixes it and also moves sorting from the user to the library (from vim/plugins/update.py to pluginupdate.py) to prevent such weird issues and duplication of code.

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.

CC @NixOS/neovim


Add a 👍 reaction to pull requests you find important.

@GaetanLepage
Copy link
Contributor

Thank you for this !
Could you please isolate the source code formatting in a separate commit please ? This would help with the review.

Ruff - an extremely fast Python linter and code formatter, written in Rust.
I fixed many hidden bugs and made some small improvements. The main
reason this was separated from NixOS#336137 is to fix the sorting issue.

Before this commit, sorting for Vim plugins was broken and worked by
what was fetched first. This is because the sorting was done by empty
strings (the default value in CSV is not None but an empty string). This
PR fixes it and also moves sorting from the user to the library (from
`vim/plugins/update.py` to `pluginupdate.py`) to prevent such weird
issues and duplication of code.
@PerchunPak PerchunPak force-pushed the fixes-for-pluginsupdate branch from cb15b92 to e9b1d2d Compare November 8, 2024 23:22
sys.path.insert(
0, os.path.join(ROOT.parent.parent.parent.parent.parent, "maintainers", "scripts")
)
import pluginupdate

GET_PLUGINS = f"""(with import <localpkgs> {{}};
GET_PLUGINS = f"""(
with import <localpkgs> {{ }};
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 also manually formatted this with nixfmt-rfc-style

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.

Thank you for splitting out the PR, it has been much easier to review it.
Also, this is a very useful work, good job :)

@@ -50,7 +50,7 @@
lib.filterAttrs (n: v: v != null) checksums
)"""

HEADER = "# This file has been generated by ./pkgs/applications/editors/kakoune/plugins/update.py. Do not edit!"
HEADER = "# This file has been @generated by ./pkgs/applications/editors/kakoune/plugins/update.py. Do not edit!"
Copy link
Contributor

Choose a reason for hiding this comment

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

why this @ ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Just saw that the same does poetry

python-poetry/poetry#2773

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, fine by me.

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.

LGTM ! Good job :)

Copy link
Contributor

@khaneliman khaneliman left a comment

Choose a reason for hiding this comment

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

LGTM thanks for tackling this

@khaneliman khaneliman changed the title pluginupdate.py: fix bugs and add improvements pluginupdate.py: fix bugs and add improvements; vimPlugins: sort properly Nov 9, 2024
@GaetanLepage
Copy link
Contributor

nixpkgs-review result

Generated using nixpkgs-review.

Command: nixpkgs-review pr 353786


x86_64-linux

✅ 3 packages built:
  • luarocks-packages-updater
  • luarocks-packages-updater.dist
  • vimPluginsUpdater

aarch64-linux

✅ 3 packages built:
  • luarocks-packages-updater
  • luarocks-packages-updater.dist
  • vimPluginsUpdater

x86_64-darwin

✅ 3 packages built:
  • luarocks-packages-updater
  • luarocks-packages-updater.dist
  • vimPluginsUpdater

aarch64-darwin

✅ 3 packages built:
  • luarocks-packages-updater
  • luarocks-packages-updater.dist
  • vimPluginsUpdater

@teto
Copy link
Member

teto commented Nov 9, 2024

ran it locally. Seemed ok.

@teto teto merged commit a31f2a7 into NixOS:master Nov 9, 2024
29 checks passed
@teto
Copy link
Member

teto commented Nov 9, 2024

thanks for the work. Now onto single plugin updates :p
sry it took so much time but this could have been instantaneous if there were not 3 different changes bundled in the PR xD

@PerchunPak PerchunPak deleted the fixes-for-pluginsupdate branch November 10, 2024 00:34
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.

4 participants