Skip to content

Re-export core::ascii::EscapeDefault as core::num::EscapeAscii #95227

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

Closed

Conversation

clarfonthey
Copy link
Contributor

@clarfonthey clarfonthey commented Mar 23, 2022

This is required for #93887. The change does three things, namely:

  1. It moves the body of the escape_default method into the inherent escape_ascii method, since the inherent version is preferred. This mirrors what was done for the deprecated AsciiExt trait, which also just references the inherent replacements.
  2. It moves the type EscapeDefault into the core::num module, where it's exported as core::num::EscapeAscii instead. To do this, I mostly just copied the core::ascii module into core::num::ascii and renamed the type, although I also edited the Debug implementation for good measure.
  3. The original EscapeDefault type is replaced with a type alias. Regardless of what name the new version is stabilised as, this shouldn't actually affect compatibility. (todo: verify this?)

Note that the code appears slightly different in the moved module since the original was never rustfmted. Somehow, it even ended with a return value; before the change, showing its age.

@rust-highfive
Copy link
Contributor

r? @Mark-Simulacrum

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 23, 2022
@clarfonthey clarfonthey force-pushed the ascii_escape_default branch from a3c669a to 289e40c Compare March 23, 2022 03:06
@Mark-Simulacrum
Copy link
Member

The original EscapeDefault type is replaced with a type alias. Regardless of what name the new version is stabilised as, this shouldn't actually affect compatibility. (todo: verify this?)

I would expect this to be a re-export, i.e., pub use ... as EscapeDefault;.

r? rust-lang/libs-api as an API change though, since I believe EscapeAscii is new here.

@clarfonthey
Copy link
Contributor Author

The original EscapeDefault type is replaced with a type alias. Regardless of what name the new version is stabilised as, this shouldn't actually affect compatibility. (todo: verify this?)

I would expect this to be a re-export, i.e., pub use ... as EscapeDefault;.

r? rust-lang/libs-api as an API change though, since I believe EscapeAscii is new here.

I'm fine doing that, I mostly wasn't sure how that would interact with stability since I thought in some cases that could accidentally stabilise the original.

But this is mostly just vague memories of a really long time ago so I don't know what the best solution is today.

@clarfonthey clarfonthey force-pushed the ascii_escape_default branch from 289e40c to e1de24b Compare April 16, 2022 00:31
@rustbot rustbot added the T-libs Relevant to the library team, which will review and decide on the PR/issue. label Apr 16, 2022
@dtolnay dtolnay added T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. and removed T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Apr 16, 2022
Copy link
Member

@dtolnay dtolnay left a comment

Choose a reason for hiding this comment

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

I'm not sure about core::num::EscapeAscii. Contrasted with the existing stable core::slice::EscapeAscii which escapes an ASCII slice, I don't find it clear to say that this one "escapes an ASCII number". The standard library's use of 'number' refers to arithmetic objects, not atoms of text. For atoms of text it tends to call them 'byte'. For example consider the difference between str.as_bytes() vs str.as_numbers().

Mentioning @rust-lang/libs-api in case someone has a preference for this or any of the alternatives listed in #77174 (comment): core::char, core::str, or one I'll add: core::u8::EscapeAscii, or that we should keep a core::ascii module, or add core::byte or core::bytes, or …

@dtolnay dtolnay added S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 16, 2022
@clarfonthey
Copy link
Contributor Author

I agree that core::num is a very weird place to put it, but at least since almost all of the reasons for the integer-type-specific modules to exist have been removed, I didn't want to put it in core::u8 either.

I actually kind of like the idea of a core::byte module, since it naturally parallels the core::char module and I could see it also containing other stuff in the future. However, I'm not sure how much…

Since the end result is unstable either way, my main intent was to just lay most of the groundwork that would be required for renaming this type, without actually committing to it. However, if we do want to move it to core::ascii::EscapeAscii, then I guess some of that work wouldn't be as useful. 🤷🏻

@m-ou-se
Copy link
Member

m-ou-se commented Apr 20, 2022

core::fmt could also work.

Copy link
Member

@dtolnay dtolnay left a comment

Choose a reason for hiding this comment

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

We discussed this PR in today's T-libs-api meeting and our impression was that we would prefer to keep core::ascii for now. core::num was not favorable to anyone, and there was a vague sense that core::byte or core::bytes would be valuable (to avoid overloading core::u8 to mean both the "number" u8 and the "byte" u8), but not if this iterator type is the only thing in there. We would be open to moving into core::byte in the future if other things arise that sensibly belong in that module. core::str was discussed but this iterator type has nothing to do with the type str, core::char was discussed but this has almost nothing to do with the type char, and core::fmt was discussed but this type has nothing to do with the Formatter API, so overall none of those were seen as sufficiently compelling over keeping core::ascii.

I'll close this PR, but we would still be interested in just the documentation parts of this PR, so I would be happy to accept a new PR containing just those. In particular treating https://doc.rust-lang.org/1.60.0/core/primitive.u8.html#method.escape_ascii as the place where the behavior is described in detail, and having https://doc.rust-lang.org/1.60.0/core/ascii/fn.escape_default.html link to it, instead of vice versa; and having https://doc.rust-lang.org/1.60.0/core/ascii/struct.EscapeDefault.html indicate that it is constructed by the inherent method, not the function.

Thanks!

@dtolnay dtolnay closed this Apr 20, 2022
@clarfonthey clarfonthey deleted the ascii_escape_default branch April 21, 2022 16:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants