rust: use split_once where appropriate
#4950
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Summary: Rust 1.52 is out! The [release notes] have the rundown, but I’m excited in particular for [`str::split_once`] and a [caching fix for Clippy]. [release notes]: https://blog.rust-lang.org/2021/05/06/Rust-1.52.0.html [`str::split_once`]: https://doc.rust-lang.org/std/primitive.str.html#method.split_once [caching fix for Clippy]: rust-lang/rust-clippy#4612 wchargin-branch: rust-v1.52.0 wchargin-source: b4be6db4e642e3c2bbf34d476556901cf12c0c60
Summary: We have a small amount of logic to parse `--logdir` flag values, to determine whether they represent a path on disk or a GCS bucket/prefix. This patch factors out the pure parsing logic from the construction of the GCS credentials and HTTP client, adding unit tests for the parsing. Test Plan: Newly added unit tests suffice. wchargin-branch: rust-logdir-parsing-tests wchargin-source: e8613fb475a8598af0e8a3802f6e1bd0d895c195
Summary: In #4793, we lamented that [`str::split_once`] was unstable, but it’s stabilized as of Rust 1.52.0, so we can simplify the protocol parsing code a tiny bit. [`str::split_once`]: https://doc.rust-lang.org/std/string/struct.String.html#method.split_once Test Plan: The recently added unit tests should suffice, but end-to-end tests with the following `--logdir`s also don’t hurt: - a local path - a valid `gs://path` - a `notgs://` path (falls back to non-RustBoard) - a `not_protocol://` path (interpreted as a path on disk) wchargin-branch: rust-split-once wchargin-source: ae1cbbcb2596e0f63884912223dc865651823a45
wchargin-branch: rust-logdir-parsing-tests wchargin-source: d55f4a5ed58ce6b7a11b2b470768df2de9ec4c2a
wchargin-branch: rust-split-once wchargin-source: cfb959cce3766e8b5ea9b0559a9d451e6996a2e3
wchargin-branch: rust-logdir-parsing-tests wchargin-source: 14ba10b0511bea256a4098b87de18e3217b59043
wchargin-branch: rust-split-once wchargin-source: 3ef111fe6b1c6e96facfde0b3950a4c0530c66b4
wchargin-branch: rust-logdir-parsing-tests wchargin-source: fe5ed050770e097b142bb502b3c597afaba35b12
wchargin-branch: rust-split-once wchargin-source: 1e49b07a7d0dc8c3f12b47cc1a91e12933034360
wchargin-branch: rust-split-once wchargin-source: 744bdd67218fb6b9ba39749e19af3ff7521218f1 # Conflicts: # tensorboard/data/server/cli/dynamic_logdir.rs
wchargin-branch: rust-split-once wchargin-source: 744bdd67218fb6b9ba39749e19af3ff7521218f1
nfelt
approved these changes
May 10, 2021
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary:
In #4793, we lamented that
str::split_oncewas unstable, but it’sstabilized as of Rust 1.52.0, so we can simplify the protocol parsing
code a tiny bit.
Test Plan:
The recently added unit tests should suffice, but end-to-end tests with
the following
--logdirs also don’t hurt:gs://pathnotgs://path (falls back to non-RustBoard)not_protocol://path (interpreted as a path on disk)wchargin-branch: rust-split-once