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

Add missing --hash-algo flag to nix store add #9809

Merged
merged 1 commit into from
Jan 20, 2024

Conversation

Ericson2314
Copy link
Member

Motivation

This is a paramter to the underlying addToStore that we forgot to expose

Context

Was supposed to have been part of #8915.

Priorities and Process

Add 👍 to pull requests you find important.

The Nix maintainer team uses a GitHub project board to schedule and track reviews.

@Ericson2314 Ericson2314 requested a review from edolstra as a code owner January 19, 2024 06:19
@github-actions github-actions bot added the new-cli Relating to the "nix" command label Jan 19, 2024
@github-actions github-actions bot added the with-tests Issues related to testing. PRs with tests have some priority label Jan 19, 2024
@grahamc
Copy link
Member

grahamc commented Jan 19, 2024 via email

@edolstra
Copy link
Member

By the way it looks like —name and —mode above are both using -n as the short flag

That looks like a cut&paste mistake. --mode doesn't need a short flag.

Copy link
Member

@thufschmitt thufschmitt left a comment

Choose a reason for hiding this comment

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

Thanks :)

EDIT: Didn't see @grahamc 's comment. --algorithm might be better indeed. It's an advanced-enough option that we can afford to be very explicit

thufschmitt added a commit that referenced this pull request Jan 19, 2024
`-n` was an alias for `--mode`, but that seems to just be a copy-paste error as it doesn't make sense.
`--mode` probably doesn't need a shorthand flag at all, so remove it.

Noticed in #9809 (comment)
@thufschmitt
Copy link
Member

-n for --mode is definitely a copy-paste error, I'll fix that in another PR

@Ericson2314
Copy link
Member Author

Ericson2314 commented Jan 19, 2024

I copied the "algo" name from elsewhere nix hash. oops it didn't match. Don't care what it is as long as its consistent.

@thufschmitt
Copy link
Member

Discussed during the Nix maintainers meeting today:

  • @roberth --algorithm makes more sense in isolation, but --algo is more consistent with the rest
  • --hash-algo is nicer than --algo and even more consistent
  • Agreement

@Ericson2314 will do the change, then it's good to go

@Ericson2314 Ericson2314 changed the title Add missing --algo flag to nix store add --hash-algo for nix store add and nix store convert Jan 20, 2024
@Ericson2314 Ericson2314 force-pushed the nix-store-add-algo branch 3 times, most recently from 43bf9c5 to 334312b Compare January 20, 2024 04:03
@Ericson2314 Ericson2314 changed the title --hash-algo for nix store add and nix store convert Add missing --hash-algo flag to nix store add Jan 20, 2024
@Ericson2314 Ericson2314 enabled auto-merge January 20, 2024 04:10
@Ericson2314 Ericson2314 merged commit 9b896bf into NixOS:master Jan 20, 2024
8 checks passed
@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/2024-01-19-nix-team-meeting-minutes-116/38837/1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
new-cli Relating to the "nix" command with-tests Issues related to testing. PRs with tests have some priority
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

5 participants