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

Fix Issue 17090 - dmd -transition=? needs quoting => make it -transition=help #9205

Merged
merged 3 commits into from
Jan 9, 2019

Conversation

wilzbach
Copy link
Member

@wilzbach wilzbach commented Jan 5, 2019

There's still a lot more that could be done:

  • create usage pages for other switches
  • Trigger a message like "See -transition=help for an overview of all valid options" if an incorrect option has been provided
  • ...

but this is a small start. I changed the ? as default in the docs as its very problematic in most shells. I added a test to ensure that it will continue to be accepted though.

@dlang-bot
Copy link
Contributor

Thanks for your pull request, @wilzbach!

Bugzilla references

Auto-close Bugzilla Severity Description
17090 normal dmd -transition=? needs quoting => make it -transition=help
19552 enhancement -transition is non-intuitive to use - the flag should list options by default

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub fetch digger
dub run digger -- build "master + dmd#9205"

@PetarKirov
Copy link
Member

I think part of the reason for the =? syntax comes from the long tradition of using /? as the help switch in dos and later windows cli programs. As far as I know =? should work in command prompt and powershell and it works in bash, but not in my fish shell. =help should everywhere, so I'm fine with changing it. Let's wait to see what other think.

{
return strcmp(p, "?") == 0 ||
strcmp(p, "h") == 0 ||
strcmp(p, "help") == 0;
Copy link
Member

Choose a reason for hiding this comment

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

On second thought, with this function I'm all in.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, just accept all three. I know I try them in sequence (for random utilities) until one of them works.

src/dmd/cli.d Outdated
@@ -471,7 +471,7 @@ dmd -cov -unittest myprog.d
$(DT native)$(DD use the architecture the compiler is running on)
)`,
),
Option("mcpu=?",
Option("mcpu=[h|help]",
Copy link
Member

Choose a reason for hiding this comment

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

Why not [?|h|help] ?

Copy link
Member

@PetarKirov PetarKirov Jan 5, 2019

Choose a reason for hiding this comment

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

Perhaps because ? needs to be quoted on some shells, but some users won't immediately figure out that they need to use -mcpu='?'. So instead, the suggestion would become ['?'|h|help].

Copy link
Member Author

Choose a reason for hiding this comment

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

Because I wanted to phase out ? as it's problematic on many shells. Anyhow I now changed it to: mcpu=h|help|?, s.t. it's still mentioned but not as first choice.

@wilzbach wilzbach added the Merge:72h no objection -> merge The PR will be merged if there are no objections raised. label Jan 6, 2019
@dlang-bot dlang-bot merged commit f600c11 into dlang:master Jan 9, 2019
@wilzbach wilzbach deleted the fix-19552 branch January 9, 2019 14:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Merge:auto-merge Merge:72h no objection -> merge The PR will be merged if there are no objections raised. Severity:Enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants