Pedantry #98
Pedantry #98
Conversation
|
Looks like all the commits from this already went in with #97 ? |
|
Wait, the GitHub UI shows 5 commits in #97, and says merged, but lowRISC:main only has 1 new commit. I’m either very dumb or some weirdness is going on with the UI. |
|
Hugo pushed to the PR after it's already in the merge queue (with only a single commit) |
This is more idomatic; variables should be cammel case.
The pname of this derivation is 'llvm-cheriot', suggesting this is a distinct package from 'llvm'. Changed the identifier to match. Note: Underscore is usually used to deliminate name from version.
This isn't the 'ot' version of python. Rather, it is a the python environment for opentitan, so ot should be part of the package name. This changes the name to reflect that.
This derivation isn't the 'ot' version of bazel. It's just bazelisk, with the upstream's bazel shell autocompletions.
Yeah, sorry, probably need to be more careful with branch reuse now we have merge queues |
|
I’m much more inclined to blame GitHub UI as it’s clearly their bug, only 1 commit was actually merged so the UI shouldn’t become incorrect if you mess with the branch afterwards :(
|
| @@ -56,7 +56,7 @@ | |||
| }) | |||
There was a problem hiding this comment.
Re. the naming for let bindings, is there a standard somewhere that camel case is preferred? Or maybe just that camel > kebab > snake? I see all kinds of stuff. Maybe we could setup a lint if there is a standard somewhere
There was a problem hiding this comment.
https://github.com/NixOS/nixpkgs/blob/master/CONTRIBUTING.md says use camel case for (non-package-name) variable names.
| @@ -81,7 +81,7 @@ | |||
| cheriot = pkgs.mkShell { | |||
There was a problem hiding this comment.
Re. b7bb917, the commit message doesn't seem to match the changes? The change is just snake -> kebab
| @@ -6,7 +6,7 @@ | |||
| ncurses5-fhs, | |||
There was a problem hiding this comment.
Re. ecea895, the commit message doesn't seem to match the changes again? This appears to just be a snake -> kebab change for the nix binding of opentitan's python environment.
I think the spirit of the message is correct though, we should probably give the naming another thought at some time. Ideally 'python' is distinct from a 'pythonEnv', but also should the pname contain the project that particular env is intended for? I'm undecided. I almost think that it would be better if we could namespace as e.g. packages.${system}.opentitan.pythonEnv, but flakes doesn't allow that output schema.
Sits atop #97