Skip to content

Various cosmetic improvements#6673

Closed
alexreg wants to merge 4 commits intorust-lang:masterfrom
alexreg:cosmetic-2
Closed

Various cosmetic improvements#6673
alexreg wants to merge 4 commits intorust-lang:masterfrom
alexreg:cosmetic-2

Conversation

@alexreg
Copy link
Contributor

@alexreg alexreg commented Feb 16, 2019

Related to the larger effort of rust-lang/rust#58036.

@rust-highfive
Copy link

rust-highfive commented Feb 16, 2019

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @Eh2406 (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

dwijnand
dwijnand reviewed Feb 16, 2019
View reviewed changes
Copy link
Contributor

@dwijnand dwijnand left a comment

Choose a reason for hiding this comment

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

Ran out of time. Some initial comments/questions.

ARCHITECTURE.md Outdated
Copy link
Contributor

@dwijnand dwijnand Feb 16, 2019

Choose a reason for hiding this comment

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

Do we prefer "lockfile" or "lock file"?

Copy link
Contributor Author

@alexreg alexreg Feb 16, 2019

Choose a reason for hiding this comment

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

I just normalised to the more common variant; otherwise ambivalent.

Copy link
Contributor

@dwijnand dwijnand Feb 16, 2019

Choose a reason for hiding this comment

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

Yeah I can believe that. I think I've been using lockfile, but I'm happy with a decision and applying it uniformly.

src/cargo/core/compiler/build_config.rs Outdated
Copy link
Contributor

@dwijnand dwijnand Feb 16, 2019

Choose a reason for hiding this comment

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

Do we prefer "doctest" or "doc test"?

src/cargo/core/compiler/build_config.rs Outdated
Copy link
Contributor

@dwijnand dwijnand Feb 16, 2019

Choose a reason for hiding this comment

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

Do we prefer doc comments to end in full stops?

dwijnand
dwijnand reviewed Feb 16, 2019
View reviewed changes
Copy link
Contributor

@dwijnand dwijnand left a comment

Choose a reason for hiding this comment

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

Finished reviewing. There are some mistakes to fix and some more comments/questions. The "E.g.," (and "I.e.,") still look wrong to me, and I'd prefer something else. Also I think we can keep git as "git" unless it starts a sentence or phrase. Finally we're a bit inconsistent on how we format GitHub URLs and URLs in general, but I don't mind what we do there.

@alexreg
Copy link
Contributor Author

alexreg commented Feb 16, 2019

@dwijnand Thanks for the review. Pushed a bunch of fixes. Hopefully we're either ready to merge now or very close. :-)

@Eh2406
Copy link
Contributor

Eh2406 commented Feb 16, 2019

@bors r? @dwijnand

@rust-highfive rust-highfive assigned dwijnand and unassigned Eh2406 Feb 16, 2019
@dwijnand
Copy link
Contributor

dwijnand commented Feb 17, 2019

@alexreg want to get CI passing? Then I'll give this a final review and hopefully/ideally land it.

@alexreg
Copy link
Contributor Author

alexreg commented Feb 17, 2019

@dwijnand Ah sorry, should be done now...

@dwijnand
Copy link
Contributor

dwijnand commented Feb 18, 2019

There was a trailing unused import, so I think this should pass now.

@dwijnand
Copy link
Contributor

dwijnand commented Feb 19, 2019

@alexreg something broke with this PR. I've repushed my test-fixing commit, which you can see on your branch, but it's not showing up here in the PR and Travis CI didn't get it either. I think you should open a new PR and hopefully it doesn't break again :)

@dwijnand dwijnand mentioned this pull request Feb 20, 2019
Merged
@dwijnand
Copy link
Contributor

dwijnand commented Feb 20, 2019

Resubmitted as #6687.

@dwijnand dwijnand closed this Feb 20, 2019
bors added a commit that referenced this pull request Feb 20, 2019
@bors
Auto merge of #6687 - dwijnand:cosmetic-2, r=dwijnand
dc83ead 
Various cosmetic improvements

Resubmit of #6673 which, somehow, broke.
cc @alexreg
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@dwijnand dwijnand dwijnand left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@dwijnand dwijnand

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@alexreg @rust-highfive @Eh2406 @dwijnand