qogir-cursors-recolored: init at 2023-10-03 #432246
Conversation
There was a problem hiding this comment.
Welcome to Nixpkgs! Thanks for packaging this software.
Please review our Commit Conventions and reword your commits appropriately; their titles must follow very specific patterns as defined in our documentation. Don't forget to squash your intermediate/fixup commits.
I've left some additional review comments below.
maintainers/maintainer-list.nix
Outdated
| @@ -17344,6 +17344,12 @@ | |||
| githubId = 96225281; | |||
| name = "Mustafa Çalışkan"; | |||
| }; | |||
| mushrowan = { | |||
| email = "roarch@proton.me"; | |||
| name = "rowan amber-jones"; | |||
There was a problem hiding this comment.
This isn't required, but are you sure you don't want to capitalise your name?
There was a problem hiding this comment.
i was going by how i have it in my git config - do you think it'd be better to capitalise it?
There was a problem hiding this comment.
nah, if you prefer lowercase, let it be
| lib.checkListOfEnum "${pname}: theme variants" availableThemeVariants themeVariants | ||
| lib.checkListOfEnum | ||
| "${pname}: catppuccin color variants" | ||
| availableColorVariants.Catppuccin | ||
| catppuccinColorVariants | ||
| lib.checkListOfEnum | ||
| "${pname}: dracula color variants" | ||
| availableColorVariants.Dracula | ||
| draculaColorVariants | ||
| lib.checkListOfEnum | ||
| "${pname}: gruvbox color variants" | ||
| availableColorVariants.Gruvbox | ||
| gruvboxColorVariants | ||
| lib.checkListOfEnum | ||
| "${pname}: original color variants" | ||
| availableColorVariants.Original | ||
| originalColorVariants |
There was a problem hiding this comment.
Perhaps there's a neater way to check these? lib.asserts.* should have a function that won't require 16 lines of input checks.
There was a problem hiding this comment.
To be honest I didnt really know what this bit was for - the vast majority of this whole PR is just an adaptation of https://github.com/TeddyBearKilla/Qogir-Cursors-Recolored. What were you thinking for how to fix this? lib.asserts.assertEachOneOf seems to do basically the same thing with the ordering of inputs slightly different (unless i'm misunderstanding) so it seems like it would be just the same length.
I thought of doing some function where we zip the *variants and available*variants lists into some list of lists (or attrsets containing both) and then mapping the check over that list which contains them, kinda pseudo map variant: (eachOneOf variant.inputVariants variant.availableVariants) [<list of attrsets containing inputvariants and availablevariants goes here...>] - but this feels like overengineering it a little, maybe i'm wrong?
| runHook postInstall | ||
| ''; | ||
|
|
||
| meta = with lib; { |
There was a problem hiding this comment.
| meta = with lib; { | |
| meta = { |
Nit: Avoid wide with scopes in attribute sets. It's alright to repeat lib.* when you need it:
{
meta = {
license = lib.licenses.publicDomain;
maintainers = with lib.maintainers; [ maintainer ];
};
}|
Another thang about commits: do not merge inside a PR branch, it must remain linear. Just rebase the branch on a fresher master commit instead |
a1a6131 to
19b0db7
Compare
|
you don't have to close the PR to edit the branch, just force-push the new commits and you're good |
currently wrestling with git issues, i think it auto closed because i did something stupid (only did a shallow clone because working with full nixpkgs history makes my pc scream, but now it's complaining about merge conflicts) i'll fix and reopen when i can lol |
|
Instead of closing, consider marking the PR as draft (the button is in the right sidebar on desktop or below the comment field on mobile) |
|
okay, i think the git history from what i was doing before is just fundamentally broken somehow, and maybe someone smarter than me would know how to fix it but i've bashed my head against it for hours now and honestly getting pretty annoyed at myself. if i rebase and force push, it wont let me reopen because apparently you cant reopen force pushed branch commits. but if i git reset to the commit from the PR and force push, i cant reopen as there's no shared history between my branch and nixpkgs master. might just leave this for now and make a new PR at some point |
|
#432364 remade it here. sorry for the timewaste |
Description of changes
qogir-cursors-recolored: init at 2023-10-03
https://github.com/TeddyBearKilla/Qogir-Cursors-Recolored
I basically copied #284617 with a couple of tweaks related to capitalisation to make it work.
My first PR so please let me know if I've messed up :)
Things done
passthru.tests.nixpkgs-reviewon this PR. See nixpkgs-review usage../result/bin/.Add a 👍 reaction to pull requests you find important.