-
Notifications
You must be signed in to change notification settings - Fork 12.7k
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
Add escape_ascii method to u8 and [u8] #73111
Conversation
(rust_highfive has picked a reviewer for you, use r? to override) |
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
I feel like if these don't have ASCII in the name somehow they are quite confusing, and I'm not entirely sure if this is necessary over asking to convert to str and escaping that (perhaps via bstr crate). |
That's very fair -- I didn't know about the That said, I do think that the remaining method in the So, basically, to me, there are really two options: 1, deprecating in favour of the bstr crate, and 2, some form of this PR.
|
Ping from triage: |
Sorry, I intentionally held off on fixing the code due to @Mark-Simulacrum's concerns about naming. Was going to wait to see if there's a consensus on that before fixing, as I can do both changes at once. |
Inherent methods seem reasonable for the ASCII-specific stuff, vs. having free-standing functions in the ASCII module. I'm going to r? @BurntSushi as this sort of deals with the whole "Unicode" thing and bstr tangentially |
@BurntSushi This is a triage bump. |
☔ The latest upstream changes (presumably #73265) made this pull request unmergeable. Please resolve the merge conflicts. |
@clarfon |
Will try to get to that soon. |
d5dc387
to
4745f42
Compare
Rebased; the current errors appear to be unrelated to this change. |
library/core/src/num/mod.rs
Outdated
/// ``` | ||
#[unstable(feature = "inherent_ascii_escape", issue = "none")] | ||
#[inline] | ||
pub fn escape_default(&self) -> ascii::EscapeDefault { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hand-wavy concern: I believe all the ascii_xxx
methods on u8
ideally should have had self
rather than &self
argument, to be consistent with all other methods. The reason for &self
I believe is historical, dating back to AsciiExt
trait.
Should we use &self
here to be consistent with other ascii method, or should we use self
, to be consistent with non-ascii methods, and with the general pattern that small things are passed by self
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree with you, am fine with either convention.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's use self
here instead of &self
. In this case there's not much value in keeping consistency with this historical 'mistake'. (Since some_byte.escape_ascii()
will work either way.)
☔ The latest upstream changes (presumably #75207) made this pull request unmergeable. Please resolve the merge conflicts. |
Thanks for working on this. I have two small comments left, see above. Other than that, this looks ready to be merged. |
Oh, one more thing: This now uses both |
Oh, yes, I did. Must have been a find-replace error. Will try and remember to get to this in a few hours when I'm done with work. |
(I changed the PR title to reflect that it adds |
Finally got to this and rebased with the feature flag fixed and suggestions applied, although I reworded the [u8] docs to be more consistent with the u8 docs. |
The job Click to see the possible cause of the failure (guessed by this bot)
|
Co-authored-by: Camelid <[email protected]>
The job Click to see the possible cause of the failure (guessed by this bot)
|
Co-authored-by: Camelid <[email protected]>
The job Click to see the possible cause of the failure (guessed by this bot)
|
Hmm, I'm not sure why the intra-doc links aren't resolving... |
Pretty sure it's because |
|
What is the status of this? |
@clarfonthey |
escape_ascii take 2 The previous PR, rust-lang#73111 was closed for inactivity; since I've had trouble in the past reopening closed PRs, I'm just making a new one. I'm still running the tests locally but figured I'd open the PR in the meantime. Will fix whatever errors show up so we don't have to wait again for this. r? `@m-ou-se`
escape_ascii take 2 The previous PR, rust-lang#73111 was closed for inactivity; since I've had trouble in the past reopening closed PRs, I'm just making a new one. I'm still running the tests locally but figured I'd open the PR in the meantime. Will fix whatever errors show up so we don't have to wait again for this. r? ``@m-ou-se``
escape_ascii take 2 The previous PR, rust-lang#73111 was closed for inactivity; since I've had trouble in the past reopening closed PRs, I'm just making a new one. I'm still running the tests locally but figured I'd open the PR in the meantime. Will fix whatever errors show up so we don't have to wait again for this. r? `@m-ou-se`
escape_ascii take 2 The previous PR, rust-lang#73111 was closed for inactivity; since I've had trouble in the past reopening closed PRs, I'm just making a new one. I'm still running the tests locally but figured I'd open the PR in the meantime. Will fix whatever errors show up so we don't have to wait again for this. r? ``@m-ou-se``
Essentially, this closes an inconsistency gap between
char
/str
andu8
/[u8]
.Right now the
EscapeDefault
type used in[u8]::escape_default
is exported incore::slice
, and it may make sense to movecore::ascii::EscapeDefault
intocore::u8
as well. But for now, this leaves thecore::ascii
module alone and just calls its methods.