Skip to content

Comments

Since --rdmd is required, made it a "non-option" argument instead of an "option that is [REQUIRED]"#335

Merged
dlang-bot merged 1 commit intodlang:masterfrom
marler8997:rdmdNonOptionArg
Mar 19, 2018
Merged

Since --rdmd is required, made it a "non-option" argument instead of an "option that is [REQUIRED]"#335
dlang-bot merged 1 commit intodlang:masterfrom
marler8997:rdmdNonOptionArg

Conversation

@marler8997
Copy link
Contributor

Non-option arguments are more conventional than "required options".

@dlang-bot
Copy link
Contributor

Thanks for your pull request and interest in making D better, @marler8997! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
Please verify that your PR follows this checklist:

  • My PR is fully covered with tests (you can see the annotated coverage diff directly on GitHub with CodeCov's browser extension
  • My PR is as minimal as possible (smaller, focused PRs are easier to review than big ones)
  • I have provided a detailed rationale explaining my changes
  • New or modified functions have Ddoc comments (with Params: and Returns:)

Please see CONTRIBUTING.md for more information.


If you have addressed all reviews or aren't sure how to proceed, don't hesitate to ping us with a simple comment.

Bugzilla references

Your PR doesn't reference any Bugzilla issue.

If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog.

@CyberShadow
Copy link
Member

Why though? --rdmd-default-compiler is still a required named argument. The actual invocation syntax also doesn't really matter because it's taken care of by makefiles.

@marler8997
Copy link
Contributor Author

You've mixed up arguments, this is for the --rdmd option, not --rdmd-default-compiler. Like it says in the description, non-option arguments are more conventional than required options.

@CyberShadow
Copy link
Member

No, I understood correctly. I'm confused why only one of the two required named arguments was converted to a positional one.

Also still needs motivation for the change. I think that when the primary invocation use case is from a script, named arguments are better than positional ones because they are unambiguously self-documenting.

@marler8997
Copy link
Contributor Author

marler8997 commented Mar 19, 2018

The plan is to make --rdmd-default-compiler optional in another PR, so doesnt make sense to make it a non-option argumemt.

It might be your opinion that

rdmd_test --rdmd rdmd

is less confusing than

rdmd_test rdmd

but for me its the opposite. Opinions aside, this goes against the convention.

@CyberShadow
Copy link
Member

The plan is to make --rdmd-default-compiler optional in another PR, so doesnt make sense to make it a non-option argumemt.

Ah, would that be the one extracted from the help text? I recall that was by itself questionable, so that leaves this PR partially based on an uncertain proposition...

but for me its the opposite. Opinions aside, this goes against the convention.

I don't think it's that unconventional. In any case, why does it matter so much to warrant a change? Hopefully no one should need to type that any more, this is now plumbing slash implementation details and the makefile is the interface intended to be used by developers.

@marler8997
Copy link
Contributor Author

Ah, would that be the one extracted from the help text? I recall that was by itself questionable, so that leaves this PR partially based on an uncertain proposition...

Not really. Whether or not that PR will get accepted does not affect this one.

I don't think it's that unconventional. In any case, why does it matter so much to warrant a change?

I'm of the opinion that "improvements" warrant change so long as they don't break anything. Following conventions is an improvement. I don't think we should be wasting our time debating over whether a non-breaking improvement is worthy of the time it takes to make the improvement itself. This is "self-defeating" behavior.

Hopefully no one should need to type that any more, this is now plumbing slash implementation details and the makefile is the interface intended to be used by developers.

No one "needs" to type it, however, the command line interface is still useful if someone would like to modify the arguments without having to change the Makefile. I modify the arguments quite a bit and use BASH history to run different variations back to back. The harder the command line interface is to use the more difficult it makes this use case.

@marler8997
Copy link
Contributor Author

I don't think it's that unconventional.

I've never seen a command line tool require a -option prefix to an argument that is always required. It's very confusing. It made me question what sort of argument could be both "optional" and be named --rdmd. Going this much against convention is confusing. Here's a list of command line tools that use "non-option" arguments for "required arguments".

cp/mv
view
less/more
cd/pushd/popd
ls
rsync
git
tee
sudo
chmod/chown
enable/disable
netstat

I don't know of any that use "-option" flags for required arguments.

…an "option that is [REQUIRED]"

Non-option arguments are more conventional than "required options".
@CyberShadow
Copy link
Member

CyberShadow commented Mar 19, 2018

I'm of the opinion that "improvements" warrant change so long as they don't break anything.

For one, we seem to disagree whether this is at all an improvement. But also, the benefit brought by the improvement must also outweigh the cost of change. For instance, this change leaves the code 6 lines longer, so it's not an improvement in code brevity!

I don't think we should be wasting our time debating over whether a non-breaking improvement is worthy of the time it takes to make the improvement itself.

Agreed, but this goes both ways. I could agree to accept this change, even though I don't think it's a substantial improvement meriting the churn, or you could agree to close it, even though you consider it a worthwhile change.

No one "needs" to type it, however, the command line interface is still useful if someone would like to modify the arguments without having to change the Makefile. I modify the arguments quite a bit and use BASH history to run different variations back to back. The harder the command line interface is to use the more difficult it makes this use case.

What's a specific example of someone needing to do that?

This is the essence of my questions above. What ultimate, non-hypothetical real-life scenario is improved by this change?

The benefits of using named arguments is obvious: they are clearer and self-documenting. The non-hypothetical real-life scenario is people looking at and trying to understand the makefile, which happens frequently enough to matter.

I'm beginning to lean towards accepting the change solely to close the argument, but perhaps you could reconsider? Or is there some crucial argument in favor of this change that hasn't been presented yet?

I've never seen a command line tool require a -option prefix to an argument that is always required. It's very confusing. It made me question what sort of argument could be both "optional" and be named --rdmd.

I'm certain I've seen tools with required named arguments before, but I can't name any off the top of my head. Yes, they are in the minority, but that's besides the point.

@marler8997
Copy link
Contributor Author

Agreed, but this goes both ways. I could agree to accept this change, even though I don't think it's a substantial improvement meriting the churn, or you could agree to close it, even though you consider it a worthwhile change.

Yes I suppose this is true. I suppose I think the change has more merit than you do. To me, options that are always "required" are very confusing for me since this goes against every other tool I've ever seen.

I also would argue that rdmd_test rdmd is just as "self-documenting" as rdmd_test --rdmd rdmd, but that would be a matter of opinion.

What's a specific example of someone needing to do that?

Any example where you'd like to run rdmd_test with different sets of arguments. Running rdmd_test back to back with different rdmd binaries, with different sets of compilers, with different models. While I was developing rdmd -i had I to do this quite a bit to test different rdmd/compiler pairs with different versions that do or do not support -i. Having to do this so often, having to type --rdmd every time is confusing, unconventional and unnecessary.

I'm certain I've seen tools with required named arguments before, but I can't name any off the top of my head. Yes, they are in the minority, but that's besides the point.

I really can't think of a single one...I've never seen this before.

Copy link
Contributor

@wilzbach wilzbach left a comment

Choose a reason for hiding this comment

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

I'm fine with this, but it's more because it's "meh" and the time discussing the pros/cons could be used to work on something cool like multiple alias this or other useful things ;-)
(Don't get me wrong, I do see your reason, but it's the testsuite that is only called via the Makefile)

Copy link
Member

@CyberShadow CyberShadow left a comment

Choose a reason for hiding this comment

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

Any example where you'd like to run rdmd_test with different sets of arguments. Running rdmd_test back to back with different rdmd binaries, with different sets of compilers, with different models. While I was developing rdmd -i had I to do this quite a bit to test different rdmd/compiler pairs with different versions that do or do not support -i. Having to do this so often, having to type --rdmd every time is confusing, unconventional and unnecessary.

OK, sounds good. Let's merge & move on.

@wilzbach
Copy link
Contributor

(though the same argument could also be made for instantly closing this and locking the discussion)

@dlang-bot dlang-bot merged commit 15cc829 into dlang:master Mar 19, 2018
@marler8997 marler8997 deleted the rdmdNonOptionArg branch March 19, 2018 13:51
@joseph-wakeling-sociomantic

(though the same argument could also be made for instantly closing this and locking the discussion)

I have to say, I don't understand why you didn't just do that. The patch is making the code objectively more complicated for no meaningful benefit ... purely an aesthetic objection to required arguments.

The fact that all the arguments were handled through flags made the code easier to understand and reason about, and the use of the arguments should only be of relevance to someone editing the makefiles.

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants