-
-
Notifications
You must be signed in to change notification settings - Fork 17.7k
ankiAddons.advanced-browser: init at 4.4 #474888
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
base: master
Are you sure you want to change the base?
Conversation
ethancedwards8
left a comment
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.
very nice work. overall lgtm
| The options to configure this add-on can be found [here](https://github.com/AnKing-VIP/advanced-browser/blob/v${finalAttrs.version}/advancedbrowser/config.md). | ||
| ''; | ||
| homepage = "https://ankiweb.net/shared/info/874215009"; | ||
| license = lib.licenses.gpl3; |
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.
| license = lib.licenses.gpl3; | |
| license = lib.licenses.gpl3Only; |
gpl3 only is discouraged nowadays. use gpl3Plus or gpl3Only. I couldn't find any info in the repo about only or later, so I think its safer/better to assume gpl3Only.
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 see, thanks a lot for reviewing, also very helpful to avoid some mistakes for the other Anki Addons I have in mind.
Applied all suggestions, from what I can get from he contributing guidelines and looking at other PRs it seems it's best practice to squash changes into the original commit, so hope the way I did it is fine.
| A general overview of the functionality can be found [here](https://ankiweb.net/shared/info/874215009). | ||
| The options to configure this add-on can be found [here](https://github.com/AnKing-VIP/advanced-browser/blob/v${finalAttrs.version}/advancedbrowser/config.md). | ||
| ''; | ||
| homepage = "https://ankiweb.net/shared/info/874215009"; |
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.
| homepage = "https://ankiweb.net/shared/info/874215009"; | |
| homepage = "https://ankiweb.net/shared/info/874215009"; | |
| downloadPage = "https://github.com/AnKing-VIP/advanced-browser"; |
| tag = "v${finalAttrs.version}"; | ||
| hash = "sha256-w2YeFHk0M2LFkyf93cy+PExTd93nu0Mk3CDGD9uu2bk="; | ||
| }; | ||
| sourceRoot = "${finalAttrs.src.name}"; |
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.
| sourceRoot = "${finalAttrs.src.name}"; |
I don't think this is necessary
| sourceRoot = "${finalAttrs.src.name}"; | ||
| passthru.updateScript = nix-update-script { }; | ||
| meta = { | ||
| description = "Advanced Browser is an Anki add-on that adds more features to the browser. It allows you to add a variety of new, sortable columns to the card browser."; |
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.
| description = "Advanced Browser is an Anki add-on that adds more features to the browser. It allows you to add a variety of new, sortable columns to the card browser."; | |
| description = "Adds more features to the browser"; |
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.
Seems I missed that, my bad. Applied, but slightly rephrased to focus that it really just adds more columns to sort by, while still staying at a concise word count.
f09a11b to
6905ed7
Compare
ethancedwards8
left a comment
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.
Lgtm. Very nice work and thank you for your contribution!
JuneStepp
left a comment
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.
There's a 4.5 version of the add-on.
Besides that, looks good!
6905ed7 to
7aa5a84
Compare
|
Great catch thank you, I only checked the Release page and did not spot that there was a tag for |
Things done
As there was discussion in an issue on home-manager about packaging more Anki addons, I decided to try packaging some of the addons I use. I have some others prepared, but since this is my first time contributing here I think it makes sense to wait if I did something massively wrong first 😅
Tested with
nix-build -E 'with import ./. {}; anki.withAddons [ ankiAddons.advanced-browser ]' && ./result/bin/anki.passthru.tests.nixpkgs-reviewon this PR. See nixpkgs-review usage../result/bin/.Add a 👍 reaction to pull requests you find important.