Skip to content

use split_once for cleaner code #12615

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Merged
merged 1 commit into from
Sep 3, 2023
Merged

use split_once for cleaner code #12615

merged 1 commit into from
Sep 3, 2023

Conversation

Eh2406
Copy link
Contributor

@Eh2406 Eh2406 commented Sep 1, 2023

What does this PR try to resolve?

Search the code base for .splitn(2 and replace with .split_once where it was clearer. I don't think any of them matter in practice.

How should we test and review this PR?

This was an internal re-factor, and the tests still pass.
The two methods have subtly different semantics, so please review carefully.

@rustbot
Copy link
Collaborator

rustbot commented Sep 1, 2023

r? @epage

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

@rustbot rustbot added A-build-execution Area: anything dealing with executing the compiler A-build-scripts Area: build.rs scripts A-cli-help Area: built-in command-line help A-registries Area: registries S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 1, 2023
(InternedString::new(path_name), Some(version))
}
Some(fragment) => match fragment.split_once([':', '@']) {
Some((name_or_version, part)) => {
Copy link
Contributor

@epage epage Sep 1, 2023

Choose a reason for hiding this comment

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

nit: I believe name_or_version is only name here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@epage
Copy link
Contributor

epage commented Sep 1, 2023

Feel free to r= me whether you decide to resolve the nit or not

let key_parts: Vec<_> = parts[0].splitn(2, ':').collect();
if parts.len() != 2 || key_parts.len() != 2 {
bail!("--man expected value with form name:1=link");
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The old code errored on having too many = or : which the new code no longer does. Do we care?

Copy link
Member

Choose a reason for hiding this comment

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

I don't fully understand the format but we can keep the behavior the same for this refactor

let key_parts: Vec<_> = parts[0].splitn(2, ':').collect();
if parts.len() != 2 || key_parts.len() != 2 {
bail!("--man expected value with form name:1=link");
}
Copy link
Member

Choose a reason for hiding this comment

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

I don't fully understand the format but we can keep the behavior the same for this refactor

@weihanglo
Copy link
Member

@bors r=epage

@bors
Copy link
Contributor

bors commented Sep 3, 2023

📌 Commit 1ea81a3 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 3, 2023
@bors
Copy link
Contributor

bors commented Sep 3, 2023

⌛ Testing commit 1ea81a3 with merge cff6d19...

@bors
Copy link
Contributor

bors commented Sep 3, 2023

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

@bors bors merged commit cff6d19 into rust-lang:master Sep 3, 2023
@Eh2406 Eh2406 deleted the split_once branch September 4, 2023 01:57
bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 6, 2023
Update cargo

21 commits in 96fe1c9e1aecd8f57063e3753969bb6418fd2fd5..d14c85f4e6e7671673b1a1bc87231ff7164761e1
2023-08-29 20:10:34 +0000 to 2023-09-05 22:28:10 +0000
- fix(resolver): Make resolver behavior independent of package order (rust-lang/cargo#12602)
- cargo-credential: change serialization of cache expiration (rust-lang/cargo#12622)
- Update registry-web-api.md yank/unyank comments (rust-lang/cargo#12619)
- test: new options of debuginfo are no longer unstable (rust-lang/cargo#12618)
- use split_once for cleaner code (rust-lang/cargo#12615)
- stop using lazy_static (rust-lang/cargo#12616)
- doc: adjust all doc headings one level up (rust-lang/cargo#12595)
- chore(deps): update compatible (rust-lang/cargo#12609)
- chore(deps): update rust crate cargo_metadata to 0.17.0 (rust-lang/cargo#12610)
- Prepare for partial-version package specs (rust-lang/cargo#12591)
- refactor: Use more serde_untagged (rust-lang/cargo#12581)
- fix(cli): Help users know possible `--target` values (rust-lang/cargo#12607)
- Tab completion for --target uses rustup but fallsback to rustc (rust-lang/cargo#12606)
- Fewer temporary needless strings (rust-lang/cargo#12604)
- fix(help): Provide better commands heading for styling (rust-lang/cargo#12593)
- fix(update): Clarify meaning of --aggressive as --recursive (rust-lang/cargo#12544)
- docs(changelog): Clarify language for Cargo.lock policy (rust-lang/cargo#12601)
- fix typo: "default branch branch" -&gt; "default branch" (rust-lang/cargo#12598)
- fix: add error for unsupported credential provider version (rust-lang/cargo#12590)
- fix(help): Explain --explain (rust-lang/cargo#12592)
- fix(help): Remove redundant information from new/init (rust-lang/cargo#12594)

r? ghost
@ehuss ehuss added this to the 1.74.0 milestone Sep 30, 2023
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
A-build-execution Area: anything dealing with executing the compiler A-build-scripts Area: build.rs scripts A-cli-help Area: built-in command-line help A-registries Area: registries 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.

6 participants