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.

Sign up for GitHub

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

Jump to bottom

Clarify the debug-related values should take boolean #77766

Merged
merged 1 commit into from
Oct 10, 2020
Merged

Clarify the debug-related values should take boolean #77766

merged 1 commit into from
Oct 10, 2020

Conversation

JohnTitor
Copy link
Member

#76588 tweaked their placeholders but these values should take boolean and the current placeholders are confusing, at least for me.

@rust-highfive
Copy link
Collaborator

r? @Mark-Simulacrum

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 9, 2020
@jyn514
Copy link
Member

jyn514 commented Oct 9, 2020 •
edited
Loading

Hmm, I don't know if I like this change. You've changed the default to false, the default of debug, but that is not the default value here: the default is the value of debug. If you set debug = true, these will default to true, not to false.

@jyn514
Copy link
Member

jyn514 commented Oct 9, 2020

r? @jyn514

@jyn514 jyn514 added the T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) label Oct 9, 2020
@tmiasko
Copy link
Contributor

tmiasko commented Oct 10, 2020

This is being changed back and forth. See for example previous discussion in #73538.

@JohnTitor
Copy link
Member Author

@jyn514 Okay, you think a placeholder should be a "default value" but I thought it should've been a "taken value" :)
Hmm, but it's not obvious to me that they take boolean since their docs aren't mentioned. Actually, I just uncommented debug-assertions then x.py complained that the value was wrong, it made me confused. So I'd like to improve in some way.

@jyn514
Copy link
Member

jyn514 commented Oct 10, 2020

I would like to avoid changing this back and forth. If we do change this, I would like a policy somewhere (maybe just at the top of config.toml?) so people know not to change this back.

That said, I'm not strongly opposed to this change as long as it's documented that it's not the default. Currently config.toml says these are defaults, so this would break that: https://github.com/rust-lang/rust/blob/03eb74a3c833778a38a476c36ece8bbfd2cdfbc1/config.toml.example#L6-L7

@JohnTitor
Copy link
Member Author

@jyn514 Fair enough! So what about this?

#debug-assertions = rust.debug value (boolean)

It doesn't lose the current meaning and tells the users that the value should take boolean.

@jyn514
Copy link
Member

jyn514 commented Oct 10, 2020

Sure, that sounds good to me. I would remove "value" in the comment though, or maybe change it to "value of rust.debug".

@JohnTitor
Clarify the debug-related values should take boolean
2224e26 
They should take boolean values and the current placeholders are confusing, at least for me.
@JohnTitor JohnTitor changed the title Fix debug-related config's values to boolean Clarify the debug-related values should take boolean Oct 10, 2020
@JohnTitor
Copy link
Member Author

@jyn514 Updated.

@jyn514
Copy link
Member

jyn514 commented Oct 10, 2020

@bors r+ rollup

Thanks for bringing this up :) I can definitely see how it was confusing before.

@bors
Copy link
Contributor

bors commented Oct 10, 2020

📌 Commit 2224e26 has been approved by jyn514

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 10, 2020
JohnTitor added a commit to JohnTitor/rust that referenced this pull request Oct 10, 2020
@JohnTitor
Rollup merge of rust-lang#77766 - JohnTitor:fix-debug-config, r=jyn514
991583e 
Clarify the debug-related values should take boolean

rust-lang#76588 tweaked their placeholders but these values should take boolean and the current placeholders are confusing, at least for me.
@JohnTitor JohnTitor mentioned this pull request Oct 10, 2020
Closed
@JohnTitor JohnTitor mentioned this pull request Oct 10, 2020
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Oct 10, 2020
@bors
Auto merge of rust-lang#77798 - JohnTitor:rollup-82u711m, r=JohnTitor
790d19c 
Rollup of 10 pull requests

Successful merges:

 - rust-lang#77195 (Link to documentation-specific guidelines.)
 - rust-lang#77629 (Cleanup of `eat_while()` in lexer)
 - rust-lang#77709 (Link Vec leak doc to Box)
 - rust-lang#77738 (fix __rust_alloc_error_handler comment)
 - rust-lang#77748 (Dead code cleanup in windows-gnu std)
 - rust-lang#77754 (Add TraitDef::find_map_relevant_impl)
 - rust-lang#77766 (Clarify the debug-related values should take boolean)
 - rust-lang#77777 (doc: disambiguate stat in MetadataExt::as_raw_stat)
 - rust-lang#77782 (Fix typo in error code description)
 - rust-lang#77787 (Update `changelog-seen` in config.toml.example)

Failed merges:

r? `@ghost`
@bors bors merged commit 95d4215 into rust-lang:master Oct 10, 2020
@rustbot rustbot added this to the 1.49.0 milestone Oct 10, 2020
@JohnTitor JohnTitor deleted the fix-debug-config branch October 10, 2020 21:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@jyn514 jyn514

Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap)
Projects
None yet
Milestone
1.49.0
Development

Successfully merging this pull request may close these issues.
7 participants
@JohnTitor @rust-highfive @jyn514 @tmiasko @bors @Mark-Simulacrum @rustbot