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

refactor: put Source trait under cargo::sources #12527

Merged
merged 2 commits into from
Sep 7, 2023
Merged

refactor: put Source trait under cargo::sources #12527

merged 2 commits into from
Sep 7, 2023

Conversation

weihanglo
Copy link
Member

What does this PR try to resolve?

It feels more natural that Source trait is under cargo::sources, as that module contains all built-in implementaions of Source trait.

This PR also flattens the module path of SourceId.

How should we test and review this PR?

Should be safe since this doesn't include any behavior change.

Additional information

This was a something in my git stash for a while when I trie extracting the real "core" types into a sane module that doesn't contain any source downloading logic.

@rustbot
Copy link
Collaborator

rustbot commented Aug 18, 2023

r? @epage

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

@rustbot rustbot added A-dependency-resolution Area: dependency resolution and the resolver A-directory-source Area: directory sources (vendoring) A-future-incompat Area: future incompatible reporting A-git Area: anything dealing with git A-infrastructure Area: infrastructure around the cargo repo, ci, releases, etc. A-interacts-with-crates.io Area: interaction with registries A-overrides Area: general issues with overriding dependencies (patch, replace, paths) A-registries Area: registries A-source-replacement Area: [source] replacement Command-add Command-install Command-publish S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 18, 2023
@epage
Copy link
Contributor

epage commented Aug 21, 2023

I have mixed feelings about this.

Overall, our layering is a mess in that core and sources depend on each other. I'd love to be findign ways to make the layering more strict as i feel that will help make the code more understandable in the short term and easier to split up into smaller, more understandable chunks in the long term.

Most likely this kind of layering would involve keeping the trait where it is and having a way to inject the implementations into core.

But maybe we are too far off from that to be worth designing towards? And having all source related stuff in one place could be nice?

What are your thoughts @weihanglo ?

@weihanglo
Copy link
Member Author

What I was trying to do is keep the "core" types being pure types, so that other modules importing them don't need to pull in unrelated code. For example, SourceId can be decoupled from Source trait, so that if we had a lockfile processing module, it doesn't need to depend on Source trait and its family.

SourceId::load is an outlier of that, though we can turn it a SourceIdExt it under cargo::sources. People want that extension can import it at will.

Not sure if it makes sense though.

@bors
Copy link
Contributor

bors commented Aug 25, 2023

☔ The latest upstream changes (presumably #12553) made this pull request unmergeable. Please resolve the merge conflicts.

@weihanglo weihanglo removed A-git Area: anything dealing with git Command-publish A-overrides Area: general issues with overriding dependencies (patch, replace, paths) Command-install A-interacts-with-crates.io Area: interaction with registries A-dependency-resolution Area: dependency resolution and the resolver A-registries Area: registries A-source-replacement Area: [source] replacement A-directory-source Area: directory sources (vendoring) A-future-incompat Area: future incompatible reporting labels Aug 26, 2023
@rustbot rustbot added A-dependency-resolution Area: dependency resolution and the resolver A-directory-source Area: directory sources (vendoring) A-future-incompat Area: future incompatible reporting A-git Area: anything dealing with git A-infrastructure Area: infrastructure around the cargo repo, ci, releases, etc. A-interacts-with-crates.io Area: interaction with registries A-overrides Area: general issues with overriding dependencies (patch, replace, paths) A-registries Area: registries A-source-replacement Area: [source] replacement Command-add Command-install Command-publish labels Sep 3, 2023
@bors
Copy link
Contributor

bors commented Sep 6, 2023

☔ The latest upstream changes (presumably #12629) made this pull request unmergeable. Please resolve the merge conflicts.

@epage
Copy link
Contributor

epage commented Sep 7, 2023

Eh, my main concern is how this helps us get to long term goals but those are probably far enough away that its not worth blocking this on

@bors r+

@bors
Copy link
Contributor

bors commented Sep 7, 2023

📌 Commit e575448 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 Sep 7, 2023
@bors
Copy link
Contributor

bors commented Sep 7, 2023

⌛ Testing commit e575448 with merge 24840d9...

@bors
Copy link
Contributor

bors commented Sep 7, 2023

☀️ Test successful - checks-actions
Approved by: epage
Pushing 24840d9 to master...

@bors bors merged commit 24840d9 into rust-lang:master Sep 7, 2023
19 checks passed
This was referenced Sep 7, 2023
Closed
Merged
@weihanglo weihanglo mentioned this pull request Sep 9, 2023
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 9, 2023
@bors
Auto merge of rust-lang#115707 - weihanglo:update-cargo, r=weihanglo
bfcc360 
Update cargo

14 commits in d14c85f4e6e7671673b1a1bc87231ff7164761e1..2fc85d15a542bfb610aff7682073412cf635352f
2023-09-05 22:28:10 +0000 to 2023-09-09 01:49:46 +0000
- feat: Stabilize lints (rust-lang/cargo#12648)
- Ues strip_prefix for cleaner code (rust-lang/cargo#12631)
- fix: don't print _TOKEN suggestion when not applicable (rust-lang/cargo#12644)
- Bump cargo-credential-1password to v0.4.0 (rust-lang/cargo#12641)
- refactor: put `Source` trait under `cargo::sources` (rust-lang/cargo#12527)
- Error out if `cargo clean --doc` is mixed with `-p`. (rust-lang/cargo#12637)
- Add wrappers around std::fs::metadata (rust-lang/cargo#12636)
- Add with_stdout_unordered. (rust-lang/cargo#12635)
- Fix example for creating a git project test. (rust-lang/cargo#12632)
- Read/write the encoded `cargo update --precise` in the same place (rust-lang/cargo#12629)
- docs(guide): Apply feedback on CI (rust-lang/cargo#12630)
- fix: improve warning for both token & credential-provider (rust-lang/cargo#12626)
- Skip clean up `profile.release.package."*"` (rust-lang/cargo#12624)
- Add MSRV validation GitHub Action for cargo-credential (rust-lang/cargo#12623)
@ehuss ehuss added this to the 1.74.0 milestone Sep 30, 2023
@weihanglo weihanglo deleted the refactor-source-trait branch October 20, 2023 21:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@epage epage

Labels
A-cargo-api Area: cargo-the-library API and internal code issues A-dependency-resolution Area: dependency resolution and the resolver A-directory-source Area: directory sources (vendoring) A-future-incompat Area: future incompatible reporting A-git Area: anything dealing with git A-infrastructure Area: infrastructure around the cargo repo, ci, releases, etc. A-interacts-with-crates.io Area: interaction with registries A-overrides Area: general issues with overriding dependencies (patch, replace, paths) A-registries Area: registries A-source-replacement Area: [source] replacement Command-add Command-install Command-publish S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Milestone
1.74.0
Development

Successfully merging this pull request may close these issues.
5 participants
@weihanglo @rustbot @epage @bors @ehuss