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

Rename ddev get to ddev addon with sub-commands #6146

Open
1 task done
GuySartorelli opened this issue May 1, 2024 · 18 comments
Open
1 task done

Rename ddev get to ddev addon with sub-commands #6146

GuySartorelli opened this issue May 1, 2024 · 18 comments

Comments

@GuySartorelli
Copy link
Collaborator

GuySartorelli commented May 1, 2024

Is there an existing issue for this?

  • I have searched the existing issues

Is your feature request related to a problem?

ddev get isn't immediately obvious by the name what it does. What's more, ddev get --list and ddev get --installed, an especially ddev get --remove are all semantically a bit strange.

Having the command be ddev addon would be more immediately obvious what the command is for, and splitting the semantically different actions into sub commands means you don't have to worry about what happens if someone tries something weird like ddev get --list --all --installed --remove my-addon or just ddev get --all

"get" is also a very common word which makes it harder to search for, e.g. there's 19 results for "get" in the docs (only one is about this command) vs 3 for "addon", and searching "get" and "addon" in github issues similarly shows a lot more results for "get" which are just issues where that word happened to be used, not at all related to the concept of addons nor about the get command.

Describe your solution

Add a new ddev addon command, with sub commands for each semantically different action (get to get an addon, remove to remove one, list to list them)

ddev addon get my-addon
ddev addon remove my-addon
ddev addon list --all
ddev addon list --installed

Then deprecate ddev get, and update it so all it does is call the appropriate ddev addon command. Eventually, remove ddev get in a future release.

I would suggest also hiding ddev get so it doesn't appear in documentation or in ddev help, ddev --help, etc. But that could be done later on closer to the time it gets removed, if there's any concern about people wondering where it went.
If it's not hidden, its description should be updated to indicate that it's deprecated and to use the new command instead.

Proposed structure of the new command

The addon command would have three sub-commands: list, get, and remove.

0ld new
ddev get --list ddev addon list
ddev get --list --all ddev addon list --all
ddev get --installed ddev addon list --installed
ddev get my-addon ddev addon get my-addon
ddev get --remove my-addon ddev addon remove my-addon

Describe alternatives

The command could be called add-on instead of addon if that's preferable, since the documentation broadly refers to "add-ons". Personally I prefer addon because it's nicer to type - and I think it's obvious that the addon command is related to what the documentation broadly refers to as "add-ons". But either way is fine.

Additional context

No response

@rfay
Copy link
Member

rfay commented May 1, 2024

It's a nice idea, but we needed you to comment on it a couple years ago :)

@GuySartorelli
Copy link
Collaborator Author

I agree it would have been good if it was always this way. But since it wasn't, I think having both commands for a short period, with the current one being deprecated for a while, is a good way forward.

If DDEV currently doesn't have a way to support introducing changes to functionality in a way that requires eventual-breaking-changes, then perhaps a separate discussion can be held about how to deal with that? But given there's a known way to deprecate commands, and given DDEV has done similar things to this in the past (look at ddev launch -p for example), I can't see any reason right now why this wouldn't be doable?

@rfay
Copy link
Member

rfay commented May 1, 2024

It's true we might be able to do it as an alias for a year or two and then deprecate. We've done that before.

@GuySartorelli
Copy link
Collaborator Author

Before I start working on a PR, is that something that is likely to be accepted?

@rfay
Copy link
Member

rfay commented May 1, 2024

The conversation is just an hour old. Please let it be for at least a couple of days so people could think about it.

And please expand your proposal to say how you'd change all the various sub-commands, and how you'd do a staged change. Keep all the flags the same? Change some? Would both "get" and "addon" be visible in ddev and ddev -h ?

How about the fact that we standardized on "add-on" a long time ago, how does that fit in? Change it everywhere? add-on is hard to write but perhaps better English.

@GuySartorelli
Copy link
Collaborator Author

please expand your proposal to say how you'd change all the various sub-commands

I don't think the functionality needs to change - this is mostly about naming and structuring of the command. But I will explicitly state which flags I think should become which sub-commands in the issue description.

Would both "get" and "addon" be visible in ddev and ddev -h ?

I would prefer ddev get to be hidden, but ultimately you'd be the one to decide yay or nay on that, I think. It is pitched as an option in the description already but I'll make that more explicit.

How about the fact that we standardized on "add-on" a long time ago, how does that fit in? Change it everywhere? add-on is hard to write but perhaps better English.

If you want the command to be named add-on instead of addon that's fine. Ultimately you'd decide that.

@GuySartorelli
Copy link
Collaborator Author

GuySartorelli commented May 1, 2024

The conversation is just an hour old. Please let it be for at least a couple of days so people could think about it.

I do apologise if it seemed like I was trying to rush you into making a decision - your original response didn't include any indication that you had any concerns or that there was additional thinking to be done, so I assumed that you had already made a decision as to whether it was worth pursuing.

@rpkoller
Copy link
Collaborator

rpkoller commented May 1, 2024

from a comprehension perspective i would prefer addon over get (probably also addon over add-on , the shorter and less complex the better).with the command addon you are setting the context what this command is about, while with get that isn't directly clear. and with get just based on the label i would assume it is only about downloading/getting something. but with get you are not only able to get the list of addons available, you are also able to list the addons installed for this project plus you are able to actually remove addons from a project (the exact opposite of getting something).

So a +1 for the addition or better the rename to addon. two thoughts, probably out of scope for this issue, but while adding the addon command maybe thinking about to add the option to update all the installed addons with something like ddev addon --update might be worth a thought. second, would it be a reasonable thought to avoid two flags in a row like for example ddev get --list --all. instead have one flag for the official list and one separate flag for the entire list. having two flags always puzzles me and makes me think for a second. separate flag might be clearer imho.

@GuySartorelli
Copy link
Collaborator Author

GuySartorelli commented May 1, 2024

maybe thinking about to add the option to update all the installed addons with something like ddev addon --update might be worth a thought

That sounds like a good enhancement - but I think that could easily be done as follow-on work to keep the focus of this issue specifically about the name and structure of the command. Keeping the functionality as-is will make that process a lot easier and reduce the complexity we need to think about for this change.

If that is considered for the current proposed change, I'd recommend update be a subcommand, so you'd run ddev addon update my-addon to update a specific addon, or ddev addon update --all to update all addons.

would it be a reasonable thought to avoid two flags in a row like for example ddev get --list --all.

That's part of this proposed change - ddev get --list --all would become ddev addon list --all, so you're running the list subcommand for the addon command, and passing the all flag. List all addons.
Whereas just the regular ddev addon list (currently ddev get --list) would list only the official addons. I.e. official is the default set, and you can optionally list them all.

instead have one flag for the official list and one separate flag for the entire list

Having a separate --official flag would either be redundant, or it would make it unclear what the default behaviour of ddev addon list should be. I think leaving the default to be listing official addons makes sense but if there's a suitable alternative then that's good too.

@rpkoller
Copy link
Collaborator

rpkoller commented May 2, 2024

If that is considered for the current proposed change, I'd recommend update be a subcommand, so you'd run ddev addon update my-addon to update a specific addon, or ddev addon update --all to update all addons.

as i've said i've considered my suggestion completely out of scope. i've just wanted to raise the idea in the context of the get/addon command (since it sounded like a good fit here). and about your suggestion, why not make the update for all installed addons implicit to the ddev addon update instead, that would be in line with the pattern for example homebrew or composer uses. running just update updates "everything" while when you run ddev addon update my-addon you only check and in case there is a new version update the given addon.

That's part of this proposed change - ddev get --list --all would become ddev addon list --all, so you're running the list subcommand for the addon command, and passing the all flag. List all addons.

oh my bad I've then completely misunderstood the table (just one note, the old example is then missing two dashes for ddev get list --all- based on that line i made faulty assumptions). But then i am totally fine with your suggestion if it is ddev addon list and ddev addon list --all. no need for the "offical" i've suggested

the only nitpick in regards of consistency, would it make sense to change your proposed command from ddev remove my-addon to ddev addon remove my-addon that way it would be more consistent with the rest of the commands

@GuySartorelli
Copy link
Collaborator Author

just one note, the old example is then missing two dashes for ddev get list --all based on that line i made faulty assumptions

Updated, thanks for pointing that out.

the only nitpick in regards of consistency, would it make sense to change your proposed command from ddev remove my-addon to ddev addon remove my-addon that way it would be more consistent with the rest of the commands

Yes it absolutely would lol. That was a typo, I've fixed it now. Thanks for pointing that out.

@penyaskito
Copy link
Member

penyaskito commented May 3, 2024

Maybe this is a bit out of scope, but as we are tracking the installed add-ons, might be good to also be able to "rename/set-url" addons (think of "git remote set-url").
E.g. if I installed penyaskito/ddev-hugo, and it's promoted to ddev/ddev-hugo some day, a way to update and set the new url without having to uninstall and install again.

ddev addon --seturl penyaskito/ddev-hugo ddev/ddev-hugo: re-tracks the new location and updates the plugin

@rfay
Copy link
Member

rfay commented May 3, 2024

if I installed penyaskito/ddev-hugo, and it's promoted to ddev/ddev-hugo some day, a way to update and set the new url without having to uninstall and install again.

I'm pretty sure this already works fine, for better or for worse.

@tyler36
Copy link
Collaborator

tyler36 commented May 7, 2024

I find myself typing ddev addon ... every other week so I thinks this is a great idea!
I thinking going the alias route is probably best; we're more likely to get feedback on which people prefer.

I wish addon was only 3 letter though. 😄

@GuySartorelli
Copy link
Collaborator Author

I wish addon was only 3 letter though. 😄

You can pretend it's 3 letters if you do ddev add<TAB> ;p

@rfay
Copy link
Member

rfay commented May 10, 2024

I think this idea has potential, and that we could ease it in over a few versions.

@GuySartorelli
Copy link
Collaborator Author

When you say ease it in, how do you think we should do that? Should the get command be made deprecated and hidden right away? Or should it only be deprecated at first? Or should that be left for a future version, and initially there's just two commands that are equally valid?

@stasadev
Copy link
Member

I think we can start with alias, then wait until most DDEV users have that alias available (i.e. wait a few months or quarters).

And only then start thinking about deprecation, readme add-on updates, and so on.

The biggest concern about deprecation is that users have their own non-official add-ons, so we can't just deprecate it immediately because that would add even more confusion.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants