Skip to content

make commented examples use valid syntax, and be more consistent - #73538

Merged
bors merged 5 commits into
rust-lang:masterfrom
tshepang:fix-example
Jun 26, 2020
Merged

make commented examples use valid syntax, and be more consistent #73538
bors merged 5 commits into
rust-lang:masterfrom
tshepang:fix-example

Conversation

@tshepang

@tshepangtshepang commented Jun 20, 2020

Copy link
Copy Markdown
Member

No description provided.

@rust-highfive

rust-highfive commented Jun 20, 2020

Copy link
Copy Markdown
Contributor

r? @nikomatsakis

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

@rust-highfiverust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 20, 2020
@mati865

mati865 commented Jun 20, 2020

Copy link
Copy Markdown
Member

This was purposely changed by #72382

@tshepang

tshepang commented Jun 20, 2020

Copy link
Copy Markdown
MemberAuthor

@mati865 nice catch. I added a commit to explain more what's going on (after reading the code). That is, I'd rather we have valid syntax there, than have users experience a failing build (as I did).

nikomatsakis
nikomatsakis approved these changes Jun 22, 2020
View reviewed changes

@nikomatsakisnikomatsakis left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I have observed some confusion with the current setup, where people uncomment the line and then get errors.

@nikomatsakis

nikomatsakis commented Jun 22, 2020

Copy link
Copy Markdown
Contributor

I agree that having the default values in the comments seems like a nice approach.

nikomatsakis
nikomatsakis requested changes Jun 22, 2020
View reviewed changes
Comment threadconfig.toml.example Outdated
@nikomatsakis

nikomatsakis commented Jun 22, 2020

Copy link
Copy Markdown
Contributor

I'm not sure who to loop in to get a broader range of opinions. Maybe @Mark-Simulacrum, @mark-i-m and @spastorino ? :)

mark-i-m
mark-i-m reviewed Jun 22, 2020
View reviewed changes

@mark-i-mmark-i-m left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

So I feel that the values should all be sensible defaults that one can just uncomment to use, which I think is the intent of this PR.

Moreover, I feel like this PR would be complimented well by having a config.dev.toml that has reasonable default in it that people can just cp config.dev.toml config.toml and not have to look in the file at all.

Comment threadconfig.toml.example Outdated
#
# Defaults to the Python interpreter used to execute x.py.
#python = "python"
#python = "python" # defaults to the Python interpreter used to execute x.py

This comment was marked as resolved.

Sign in to view

@nikomatsakisnikomatsakisJun 22, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Yeah, that's true, it did seem to blend in to me.

@nikomatsakis

nikomatsakis commented Jun 22, 2020

Copy link
Copy Markdown
Contributor

I do like the idea of a config.toml.dev, but I guess it's a distinct PR perhaps.

tmiasko
tmiasko reviewed Jun 23, 2020
View reviewed changes
Comment threadconfig.toml.example Outdated
Comment on lines +329 to +330
# Defaults to rust.debug value
#debug-assertions-std = false

@tmiaskotmiaskoJun 23, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This one defaults to debug-assertions and only indirectly to debug.

@spastorino

spastorino commented Jun 23, 2020

Copy link
Copy Markdown
Member

I like the approach of documenting to which value each thing defaults to but at the same time it seemed that that's what the previous code was doing already. I'm not sure what's the reason for the main change here like "examples should be of type bool".

@mark-i-m

mark-i-m commented Jun 23, 2020

Copy link
Copy Markdown
Contributor

@spastorino I think the intent is that you should be able to just uncomment a line and it should work, which is not true today IIUC.

@nikomatsakis

nikomatsakis commented Jun 23, 2020

Copy link
Copy Markdown
Contributor

I'm happy with this current version.

@nikomatsakis

nikomatsakis commented Jun 23, 2020

Copy link
Copy Markdown
Contributor

(Though we should fix the problem @tmiasko noted)

@tshepangtshepang changed the title examples should be of type boolmake commented examples use valid syntax, and be more consistent Jun 24, 2020
@tshepang

tshepang commented Jun 24, 2020

Copy link
Copy Markdown
MemberAuthor

@spastorino the PR comment got obsolete with the iterations. I updated it now.

@tshepang

tshepang commented Jun 24, 2020

Copy link
Copy Markdown
MemberAuthor

should I squash the commits

@nikomatsakis

nikomatsakis commented Jun 24, 2020

Copy link
Copy Markdown
Contributor

@bors r+ rollup

Meh, I don't think squashing is necessary.

@bors

bors commented Jun 24, 2020

Copy link
Copy Markdown
Collaborator

📌 Commit b23baa7 has been approved by nikomatsakis

@borsbors 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 Jun 24, 2020
Manishearth added a commit to Manishearth/rust that referenced this pull request Jun 26, 2020
@Manishearth
Rollup merge ofrust-lang#73538- tshepang:fix-example, r=nikomatsakis
59768d4
make commented examples use valid syntax, and be more consistent
@ManishearthManishearth mentioned this pull request Jun 26, 2020
Closed
@ManishearthManishearth mentioned this pull request Jun 26, 2020
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Jun 26, 2020
@bors
Auto merge ofrust-lang#73746- Manishearth:rollup-80jnynm, r=Manishe…
e093b65
…arth
Rollup of 14 pull requests
Successful merges:
- rust-lang#72617 (Add a fast path for `std::thread::panicking`.)
- rust-lang#72738 (Self contained linking option)
- rust-lang#72770 (Implement mixed script confusable lint.)
- rust-lang#73418 (Add unstable `core::mem::variant_count` intrinsic)
- rust-lang#73460 (Emit line info for generator variants)
- rust-lang#73534 (Provide suggestions for some moved value errors)
- rust-lang#73538 (make commented examples use valid syntax, and be more consistent )
- rust-lang#73581 (Create 0766 error code)
- rust-lang#73619 (Document the mod keyword)
- rust-lang#73621 (Document the mut keyword)
- rust-lang#73648 (Document the return keyword)
- rust-lang#73673 (Fix ptr doc warnings.)
- rust-lang#73674 (Tweak binop errors)
- rust-lang#73687 (Clean up E0701 explanation)
Failed merges:
- rust-lang#73708 (Explain move errors that occur due to method calls involving `self` (take two))
r? @ghost
@bors
bors merged commit 0d5cfb4 into rust-lang:masterJun 26, 2020
@tshepang
tshepang deleted the fix-example branch June 26, 2020 07:36
@tmiaskotmiasko mentioned this pull request Oct 10, 2020
Merged
@cuvipercuviper added this to the 1.46 milestone May 2, 2024
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@nikomatsakisnikomatsakisnikomatsakis requested changes
@tmiaskotmiaskotmiasko left review comments
+1 more reviewer
@mark-i-mmark-i-mmark-i-m left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

@nikomatsakisnikomatsakis

Labels

S-waiting-on-borsStatus: Waiting on bors to run and complete tests. Bors will change the label on completion.

Projects

None yet

Milestone

1.46

Development

Successfully merging this pull request may close these issues.

9 participants

@tshepang@rust-highfive@mati865@nikomatsakis@spastorino@mark-i-m@bors@tmiasko@cuviper