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

Make it clear the subset flag can be omitted #47691

Merged
merged 2 commits into from
Feb 23, 2021

Conversation

SingleAccretion
Copy link
Contributor

It is not necessary to explicitly specify the subset flag when building.

@joperezr
Copy link
Member

joperezr commented Feb 9, 2021

Hello @SingleAccretion, while I agree that the subset is implicit, this is only the case when it is the first argument so I believe it is intentional that our docs are in the verbose format. If anything, I think we should only mention in the subset parameter that if it is the first item, you can omit it (which we might already have this mention). @ViktorHofer thoughts?

@SingleAccretion
Copy link
Contributor Author

I suspected as much, yes. I agree that it would probably be better to just point out that the flag can be omitted, and this is indeed mentioned in various places (notably, all examples in ./build -h use the short form).

What lead me to do it this way is the fact that, having pointed a person new to dotnet/runtime to this document (and the directory in general), they did not discover this fact. Not only that, but one of the developers of dotnet/runtime did not know it either, which lead me to believe this knowledge is not as pervasive as I've assumed, which in turn lead to this PR.

Copy link
Member

@ViktorHofer ViktorHofer 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 either merging this or not but FWIW I would prefer a consistent style in our docs. I doubt that coreclr's README.md is the only place that uses the explicit subset style.

@SingleAccretion
Copy link
Contributor Author

Other docs use the explicit style. I will revert the changes and just mention the flag can be omitted.

@ViktorHofer
Copy link
Member

FWIW I think the implicit style makes more sense as I don't see a reason why the subset wouldn't be the first param.

@joperezr joperezr merged commit 9a3e866 into dotnet:master Feb 23, 2021
@joperezr
Copy link
Member

Thanks for the contribution @SingleAccretion !

@ghost ghost locked as resolved and limited conversation to collaborators Mar 25, 2021
@SingleAccretion SingleAccretion deleted the Subset-Flag-Is-Implicit branch June 8, 2021 04:12
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

3 participants