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

Some tiny refactors around ops::cargo_compile #11243

Merged
merged 5 commits into from
Oct 17, 2022

Conversation

weihanglo
Copy link
Member

What does this PR try to resolve?

Some tiny refactors I found during polishing documentations.

  • Extract CompileFilter and Packages from ops::cargo_compile to their own modules.
  • Remove FilterRule::try_collect as its intent is not clear, and we don't need this indirection.
  • Remove CompileOptions::local_rustdoc_args, which is obsolete since 1ef954e.

How should we test and review this PR?

This shouldn't include any functional change.

Better to review it commit by commit.

Additional information

Moving code back and forth is indeed bad for git history tracking, but it seems better for me to make boundaries on types with different purposes. The lines of code of cargo_compile.rs is also reduced from 1977 to 1460, which is nicer to read.

@rust-highfive
Copy link

r? @epage

(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 16, 2022
src/cargo/ops/cargo_compile.rs Show resolved Hide resolved
/// Generally, it represents the combination of all `-p` flag. When working within
/// a workspace, `--exclude` and `--workspace` flags also contribute to it.
#[derive(PartialEq, Eq, Debug)]
pub enum Packages {
Copy link
Contributor

Choose a reason for hiding this comment

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

Side note: long term, I wonder where this should belong as this seems like a more fundamental CLI package selection struct that is useful outside of compilation.

For example, clap-cargo tries to imitate this for general use. cargo-release is at least one program that uses that logic. cargo-upgrade had another implementation of it until we switched to oly operating on the workspace.

Being buried in cargo_compile makes this less discoverable.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fully agree. That's why even I hate the lost of git history so much but still did this 🫠

@weihanglo weihanglo force-pushed the refactor/cargo_compile branch from 286c5ec to 886c9d2 Compare October 17, 2022 17:59
@epage
Copy link
Contributor

epage commented Oct 17, 2022

@bors r+

@bors
Copy link
Contributor

bors commented Oct 17, 2022

📌 Commit 886c9d2 has been approved by epage

It is now in the queue for this repository.

@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 17, 2022
@bors
Copy link
Contributor

bors commented Oct 17, 2022

⌛ Testing commit 886c9d2 with merge 3ff0443...

@bors
Copy link
Contributor

bors commented Oct 17, 2022

☀️ Test successful - checks-actions
Approved by: epage
Pushing 3ff0443 to master...

@bors bors merged commit 3ff0443 into rust-lang:master Oct 17, 2022
weihanglo added a commit to weihanglo/rust that referenced this pull request Oct 18, 2022
6 commits in b332991a57c9d055f1864de1eed93e2178d49440..3ff044334f0567ce1481c78603aeee7211b91623
2022-10-13 22:05:28 +0000 to 2022-10-17 20:25:00 +0000
- Some tiny refactors around `ops::cargo_compile` (rust-lang/cargo#11243)
- Polish docs for module `build_context` (rust-lang/cargo#11241)
- Remove sparse+ prefix for index.crates.io (rust-lang/cargo#11247)
- docs(add): Add missing flags to reference (rust-lang/cargo#11240)
- Document `cargo remove` (rust-lang/cargo#11227)
- fix: Update help headings to  match clap (rust-lang/cargo#11239)
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Oct 19, 2022
Update cargo

6 commits in b332991a57c9d055f1864de1eed93e2178d49440..3ff044334f0567ce1481c78603aeee7211b91623 2022-10-13 22:05:28 +0000 to 2022-10-17 20:25:00 +0000
- Some tiny refactors around `ops::cargo_compile` (rust-lang/cargo#11243)
- Polish docs for module `build_context` (rust-lang/cargo#11241)
- Remove sparse+ prefix for index.crates.io (rust-lang/cargo#11247)
- docs(add): Add missing flags to reference (rust-lang/cargo#11240)
- Document `cargo remove` (rust-lang/cargo#11227)
- fix: Update help headings to  match clap (rust-lang/cargo#11239)
@weihanglo weihanglo deleted the refactor/cargo_compile branch October 20, 2022 03:23
@ehuss ehuss added this to the 1.66.0 milestone Oct 30, 2022
bors added a commit that referenced this pull request Feb 3, 2023
refactor: mod.rs over "name".rs for consistency

See #11243 (comment)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants