-
-
Notifications
You must be signed in to change notification settings - Fork 3k
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
change names of ipfs dag put flags to make changes clearer #8439
Conversation
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.
Looks sensible, I would shorten store-codec
to codec
to indicate it is THE codec,
but not feeling too strong either way.
cc @warpfork if any concerns (context: we need to change names to ensure people are not hit by silent bugs caused byt the change around cbor/dag-cbor and json/dag-json, which may produces different CID in some cases)
Don't feel too strongly about store-codec vs codec here, figured more text might be more informative but we could also throw that in the help text. I tried using codec in #8440 (comment). However, we can't use |
I think I'm gonna studiously not have an opinion this (at least, today). If I were going to have an opinion, I'd start forming it by sketching a new command family, and make sure I can draft the whole family at once, and make sure things are internally consistent and follow some rule. I'd draft some examples (just to get my brain working on the concretes), then document the rule (this is the important part), and only then finish implementing and do final consistency review on the commands. (And to put my time/money where my mouth is at: I started drafting some new CLI tools in another repo, where I'm going to do that, in that order. That's gonna cook for a while, though.) If we're going to do something about the And I don't know where I sit on the topic of whether it's worth changing these at all, vs just drafting generationally new APIs. My 2c is that it's often a lot easier/higher-leverage to draft generationally new things (lets you fix families of problems; also lets you maintain the old one with less change until you sunset it). But in the |
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.
Regarding the blast radius of the decision made here, AFAIK impacted commands are only dag put
and dag get
(and maybe future patch
(#4782) and diff
(#4801) operations).
I played with different notations, and IMO the value of short notation is debatable: most of the time defaults are fine, and when one needs to override them, longer notation is less prone to typos and errors are easier to catch.
👉 My suggestion is to go with clean --input-codec
and --codec
(without any shortcuts).
I concur with dropping the shortcuts/single-letter-options. I am less certain about going with |
I guess if anything is to change here, I probably agree with @ribasushi 's suggestion that things be explicit about input direction and output direction, and we pretty much tacitly never use the word "codec" bare without a directional indicator. One of the big issues with these commands and their flags as they stand now are that people get very confused which phase a codec is applied at, so if we're changing anything, it seems we should increasingly favor explicitness. Maybe some other commands only have one direction or the other, sure. But I bet it will still be clearer overall to have those commands have a longer flag just to be consistent with anything that does have flags for multiple phases of data flow. |
Latest update: We're going to use |
7201bf7
to
b83651b
Compare
Proposed changes for #8415