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

Various cosmetic improvements #6673

Closed
wants to merge 4 commits into from
Closed

Various cosmetic improvements #6673

wants to merge 4 commits into from

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

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
Member

@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.

src/bin/cargo/commands/test.rs Outdated Show resolved Hide resolved
ARCHITECTURE.md Show resolved Hide resolved
ARCHITECTURE.md
@@ -58,7 +58,7 @@ the local hard drive.

`Resolve` is the representation of a directed acyclic graph of package
dependencies, which uses `PackageId`s for nodes. This is the data
structure that is saved to the lock file. If there is no lockfile,
structure that is saved to the lock file. If there is no lock file,
Copy link
Member

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

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
Member

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.

LICENSE-THIRD-PARTY Outdated Show resolved Hide resolved
src/cargo/core/compiler/build_config.rs
pub fn is_check(self) -> bool {
match self {
CompileMode::Check { .. } => true,
_ => false,
}
}

/// Returns true if this is a doc or doctest. Be careful using this.
/// Returns `true` if this is a doc or doc test. Be careful using this.
Copy link
Member

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/bin/cargo/commands/test.rs Show resolved Hide resolved
src/cargo/core/compiler/build_config.rs
pub force_rebuild: bool,
/// Output a build plan to stdout instead of actually compiling.
pub build_plan: bool,
/// Use Cargo itself as the wrapper around rustc, only used for `cargo fix`
/// Use Cargo itself as the wrapper around rustc, only used for `cargo fix`.
Copy link
Member

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?

src/cargo/core/compiler/build_context/mod.rs Outdated Show resolved Hide resolved
src/cargo/core/compiler/build_context/target_info.rs Show resolved Hide resolved
dwijnand
dwijnand reviewed Feb 16, 2019
View reviewed changes
Copy link
Member

@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.

src/cargo/core/compiler/context/mod.rs Show resolved Hide resolved
src/bin/cargo/commands/test.rs Show resolved Hide resolved
src/cargo/core/compiler/build_context/mod.rs Outdated Show resolved Hide resolved
src/cargo/core/compiler/job_queue.rs Outdated Show resolved Hide resolved
src/cargo/core/manifest.rs Outdated Show resolved Hide resolved
src/cargo/util/command_prelude.rs Outdated Show resolved Hide resolved
tests/testsuite/build.rs Outdated Show resolved Hide resolved
tests/testsuite/package.rs Outdated Show resolved Hide resolved
tests/testsuite/package.rs Outdated Show resolved Hide resolved
tests/testsuite/package.rs Outdated Show resolved Hide resolved
@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
Member

@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
Member

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

@dwijnand
Copy link
Member

@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
Member

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

@dwijnand dwijnand dwijnand left review comments

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