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

Change help to use hyphen switches and consistent casing #3785

Merged
merged 1 commit into from Nov 14, 2018

Conversation

dasMulli
Copy link
Contributor

@dasMulli dasMulli commented Sep 23, 2018

Changes help texts and explanations in the resource file to:

  • use hyphens instead of slashes for command line arguments
  • use camelCase for all command line arguments

Fixes #3764, fixes #3765

@dasMulli dasMulli force-pushed the feature/consistent-help branch 2 times, most recently from 7bbfc37 to 593c9cc Compare September 24, 2018 00:32
@dasMulli dasMulli changed the title WIP: Change help to use hyphen switches and consistent casing Change help to use hyphen switches and consistent casing Sep 24, 2018
@dasMulli dasMulli force-pushed the feature/consistent-help branch 3 times, most recently from 0ade69a to 7e40640 Compare September 24, 2018 00:40
if (i == 0)
{
if (helpMessageLines[i].Trim().StartsWith("/") || helpMessageLines[i].Trim().StartsWith("@"))
if (trimmedLine.StartsWith("/") || trimmedLine.StartsWith("-") || trimmedLine.StartsWith("@"))
Copy link
Contributor

Choose a reason for hiding this comment

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

This unit test only tests our usage text. I think you should remove the StartsWith("/") so that we make sure all usage messages are correct

if (item.Key.Contains("Examples"))
{
// Examples require a certain number of leading spaces
Assert.True(helpMessageLines[i].StartsWith(examplesLeadingSpaces), $"Line {i + 1} of '{item.Key}' should start with '{examplesLeadingSpaces}'.");
}
else if (helpMessageLines[i].Trim().StartsWith("/") || helpMessageLines[i].Trim().StartsWith("@"))
else if (trimmedLine.StartsWith("/") || trimmedLine.StartsWith("-") || trimmedLine.StartsWith("@"))
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here

Copy link
Member

@rainersigwald rainersigwald left a comment

Choose a reason for hiding this comment

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

I think this looks good. My only question is: should we support and then suggest using --longForm arguments (with double dashes) or just stick to single-dash/slash everywhere?

I also feel like it's probably worth adding a sentence in the /? help text that says "switches can be specified with either an initial - or an initial /".

@dasMulli
Copy link
Contributor Author

… suggest using --longForm

not sure. roslyn doesn't do that.
Personally, I like -- arguments only when the single hyphen arguments are flags I can combine (like tar -xvf …)

@dasMulli dasMulli closed this Sep 24, 2018
@dasMulli dasMulli reopened this Sep 24, 2018
@dasMulli
Copy link
Contributor Author

dasMulli commented Sep 24, 2018

Added a commit with explanatory comment on switch usage:

Microsoft (R) Build Engine version 16.0.78-preview+g05baa3878b for .NET Core
Copyright (C) Microsoft Corporation. All rights reserved.

Syntax:              MSBuild.exe [options] [project file | directory]

Description:         Builds the specified targets in the project file. If
                     …

Switches:            Note that you can spefiy switches using both
                     "-switch" and "/switch".

  -target:<targets>  Build these targets in this project. Use a semicolon or a

or should this rather be in the description part?

4) all verbosity levels and their short forms e.g. quiet, or q
LOCALIZATION: None of the lines should be longer than a standard width console window, eg 80 chars.
</comment>
</data>
<data name="HelpMessage_3_SwitchesHeader" UESanitized="true" Visibility="Public">
<value>Switches:
<value>Switches: Note that you can spefiy switches using both
Copy link
Member

Choose a reason for hiding this comment

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

typo: specify

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thx, fixed

Copy link
Member

@rainersigwald rainersigwald left a comment

Choose a reason for hiding this comment

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

Thanks!

@dasMulli
Copy link
Contributor Author

Is the CLA bot expected to work? I should have a valid CLA unless it changed in the last days

@rainersigwald
Copy link
Member

CLA-bot has been crabby lately. I used my superpowers to redeliver the webhook and it turned green.

Copy link
Contributor

@AndyGerlicher AndyGerlicher left a comment

Choose a reason for hiding this comment

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

Looks really good. Thanks for taking the time to do this!

@AndyGerlicher
Copy link
Contributor

@dasMulli Just FYI I want to hold off on this as it could impact Visual Studio (there are some old tests in there). We're working on getting a stable insertion (for other reasons), once we do I'll merge this.

@dasMulli
Copy link
Contributor Author

@AndyGerlicher @rainersigwald @dfederm Or do you want this to go into exp/net472 since there are new switches (-graphBuild) being added there? Or wait until net472 is merged into master before a rebase?

@rainersigwald
Copy link
Member

I wouldn't object if you wanted to retarget to exp/net472 and fix up the new switches, but I also wouldn't mind fixing them at merge time.

@rainersigwald
Copy link
Member

Closing/reopening to re-merge and retest post-RI of net472.

@dasMulli
Copy link
Contributor Author

dasMulli commented Nov 2, 2018

Rebased and squashed the two original commits onto latest master and added additional changes for graph build command line arguments.

@dasMulli
Copy link
Contributor Author

What's the status on this? waiting for decisions or ready to merge?

@rainersigwald rainersigwald merged commit 6fa296f into dotnet:master Nov 14, 2018
@rainersigwald
Copy link
Member

I think I saw your last push and was waiting for tests or something and then forgot about it for two weeks, sorry! Thanks again.

@AndyGerlicher
Copy link
Contributor

Also for transparency, we had been trying to avoid changes that weren't strictly necessary and high potential for breaking something (this changed output messages, so might break something) while we worked to get VS updated to 4.7.2 for the first preview of VS 2019. That plus the tools version change to Current will be going out when that's released. It was a tough change since anything that referenced us or Roslyn in VS needed to update. But it should be a great win for .NET Standard components (no more facades!) and perf as well.

@dasMulli
Copy link
Contributor Author

and then forgot about it for two weeks

@rainersigwald I really appreciate that you just tell it like it is and not give some bla bla bullsh*. Not just here but in a lot of places.

we had been trying to avoid changes that weren't strictly necessary

My "open source work" is mostly things that are important to me and not necessarily high enough to land on teams' backlogs so I completely understand.

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