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

Switch verb and noun for package/reference add/remove/list Fixes #9650 #45384

Merged
merged 14 commits into from
Feb 12, 2025

Conversation

Forgind
Copy link
Member

@Forgind Forgind commented Dec 9, 2024

Fixes #9650

Switches the ordering of commands from dotnet verb noun to dotnet noun verb, as it is sometimes unclear what 'add' adds, for instance, and this is how many users of other CLIs think.

In this version, dotnet verb noun (DVN) is still supported in that it works when used, but dotnet noun verb (DNV) is the priority. DVN is now hidden and doesn't appear in the help text, whereas DNV is visible and does appear in the help text.

Incomplete:
We need to change how we accept arguments. This is still a point of discussion, so we need to build consensus on the right order before proceeding. (Possibilities include dotnet package <package> add <project>, dotnet package add <package> --project <project>, etc.)
We should consider adding a message when using DVN directing users to DNV

@JonDouglas
Copy link

image

@Forgind Forgind changed the base branch from release/9.0.2xx to release/9.0.3xx January 28, 2025 03:42
@Forgind Forgind marked this pull request as ready for review January 29, 2025 23:37
@Forgind Forgind requested review from a team as code owners January 29, 2025 23:37
@aortiz-msft aortiz-msft requested a review from OliaG January 30, 2025 21:15
@aortiz-msft
Copy link
Contributor

@OliaG , would you please review the UX changes proposed?

@aortiz-msft aortiz-msft self-requested a review January 30, 2025 21:16
@Forgind
Copy link
Member Author

Forgind commented Feb 3, 2025

Things look like they're starting to work:
image

Copy link
Member

@MiYanni MiYanni left a comment

Choose a reason for hiding this comment

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

Looks good! A majority of the changes were moved files so it wasn't so large as it seemed. Only question I had was how the docs should be updated. But we can discuss that offline with @baronfel

Comment on lines +13 to +16
public static readonly CliOption OutdatedOption = new ForwardedOption<bool>("--outdated")
{
Description = LocalizableStrings.CmdOutdatedDescription
}.ForwardAs("--outdated");
Copy link
Member

Choose a reason for hiding this comment

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

This shows my unfamiliarity with this but why does both ForwardedOption's constructor and ForwardAs have the same string for bool options?

Copy link
Member Author

Choose a reason for hiding this comment

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

Er...great question. I didn't even notice; as you pointed out, a lot of these changes were just moving things from one file to another, and this is an example. @baronfel?

Copy link
Member Author

Choose a reason for hiding this comment

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

baronfel told me that ForwardedOption is an option with a null forwarding function—that is, it doesn't actually specify how it should be forwarded, just makes that possible if you also specify ForwardAs and how to forward it.

@Forgind Forgind merged commit 4f21e8d into dotnet:release/9.0.3xx Feb 12, 2025
30 checks passed
@Forgind Forgind deleted the switch-verb-noun branch February 12, 2025 19:08
@baronfel
Copy link
Member

We'll need to doc this in the 10.0 preview 2 release notes and in the 9.0.300 changelogs (for future-Chet's reference).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-CLI Document for new feature untriaged Request triage from a team member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants