Add is_empty() for Path and OsStr #31231
Conversation
|
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @brson (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. |
frewsxcv
force-pushed
the
is_empty
branch
4 times, most recently
from
85738ba to
0dfe25d
Compare
| @@ -277,6 +277,12 @@ impl OsStr { | |||
| self.to_bytes().and_then(|b| CString::new(b).ok()) | |||
| } | |||
|
|
|||
| /// Checks if the string is empty. | |||
| #[unstable(feature = "os_extras", reason = "recently added", issue = "30259")] | |||
There was a problem hiding this comment.
Should decide between keeping #30259 as a tracking issue for this unstable feature or closing it (i.e. the “Fixes” line on the PR). I’d rather repurpose the issue in question to be a tracking issue.
Also, perhaps a more self-explanatory feature name would be better? Something like osstr_is_empty?
|
Thanks @frewsxcv! I'm tagging with |
alexcrichton
added
the
T-libs-api
| @@ -243,6 +244,7 @@ | |||
| #![feature(on_unimplemented)] | |||
| #![feature(oom)] | |||
| #![feature(optin_builtin_traits)] | |||
| #![feature(os_extras)] | |||
There was a problem hiding this comment.
Were these necessary? I thought that intra-crate usage of unstable features didn't require these annotations...
Related to rust-lang#30259 Rebase of rust-lang#30623
|
Comments have been addressed |
|
See also #29453? |
|
The libs team discussed this during triage today (sorry for taking awhile to get around to this @frewsxcv!), and we're definitely ok with this, especially in light of #29453. @frewsxcv that issue is actually tracking an RFC which broadens the methods added here to others like |
|
Sure! I'll make those changes |
|
Ok, cool, thanks! The full RFC is here, and feel free to ping me if you have any questions! |
|
Opened a new PR that covers |
|
Thanks @frewsxcv! I'm gonna close this in favor of that. |
|
@alexcrichton Should there be a RFC opened for |
|
Opened an issue there: rust-lang/rfcs#1497 |
|
Oh right, forgot about that! An RFC issue sounds good to me, though, thanks! |
Original issue requesting this feature: rust-lang#30259 Originally implemented in rust-lang#30623 but that pull request when stale. It was rebased in rust-lang#31231, but the `Path` changes got lost as the focus shifted towards `OsString` and `OsStr`. An RFC (rust-lang/rfcs#1497) was briefly opened, since I didn't know if this functionality needed an RFC, but @alexcrichton clarified in the RFC issue I linked that this is not the case.
Related to #30259
Rebase of #30623