-
-
Notifications
You must be signed in to change notification settings - Fork 14.1k
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
swaylock-plugin: init at 1.8.0 #352330
base: master
Are you sure you want to change the base?
swaylock-plugin: init at 1.8.0 #352330
Conversation
Opened a PR upstream to fix the |
7a7dd44
to
2498f76
Compare
735c4f9
to
47cb3ff
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested this in my config and it works fine.
Upstream merged the above PR, I'm considering pointing to the current main HEAD instead of the latest release 🤔 WDYT? |
I would use |
e239362
to
50116aa
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I spotted the completions being installed in the wrong place?
For example bash:
share/bash-completion/completions/bash/swaylock-plugin
instead of
share/bash-completion/completions/swaylock-plugin
maybe just fix it in postInstall
or better fix it upstream? ;)
Upstream line at fault?
https://github.com/mstoeckl/swaylock-plugin/blob/00e4f9336df974a4319592afa5a9b1ee90cfd5ca/completions/meson.build#L19
Also not required but maybe you would be willing to add versionCheckHook or testers.testVersion?
nixpkgs-review
result
Generated using nixpkgs-review
.
Command: nixpkgs-review pr 352330
x86_64-linux
✅ 1 package built:
- swaylock-plugin
50116aa
to
f2a417d
Compare
Good catch for the bash completion issue. I'll have a look at that tonight. |
f2a417d
to
1dd4c71
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
imo shell completion isn't a strict requirement. I'd suggest we just move it in postInstall
for now.. change lgtm.
Things done
nix.conf
? (See Nix manual)sandbox = relaxed
sandbox = true
nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD"
. Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/
)Add a 👍 reaction to pull requests you find important.