Skip to content
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

Implement std::ascii::AsciiExt and std::ascii::OwnedAsciiExt for Ascii #3

Closed
tomprogrammer opened this issue Jan 4, 2015 · 5 comments
Assignees

Comments

@tomprogrammer
Copy link
Owner

Motivation

Preliminary
While str guarantees statically that data of its type is valid UTF-8, the type Ascii guarantees ASCII-conformance. Therefore the types [Ascii] and str and their owned counterparts Vec<Ascii> and String should behave similar.

Topic of this Issue
Ascii provides functions like to_uppercase() and to_lowercase() which can be applied to single ascii-characters. Currently such operations are not implemented on owned or borrowed strings of ascii-characters. As the types Vec<Ascii> and [Ascii] should be opaque manually implementing the iteration isn't recommended because it is a implementation detail of these types.

Example:

error: type `&[Ascii]` does not implement any method in scope named `to_uppercase`
let _ = "abcXYZ".to_ascii().unwrap().to_uppercase();
                                     ^~~~~~~~~~~~~~~~~~~~

Design

The types String and str provide functionality for converting to uppercase and lowercase with their implementations of the traits std::ascii::{AsciiExt, OwnedAsciiExt}. These traits are intended for "[…] ASCII-subset only operations on string slices" and owned strings. Of course Vec<Ascii> and [Ascii] are subsets of ascii, they are equivalent so it's valid to implement them for the ascii only string types.

Implement the traits:

impl AsciiExt<Vec<Ascii>> for [Ascii]
impl AsciiExt for Ascii
impl OwnedAsciiExt for Vec<Ascii>

The implementations use functionality present in Ascii if possible.

Design flaws

  • The traits std::ascii::{AsciiExt, OwnedAsciiExt} are marked experimental. This shouldn't be a real issue as I expect this crate to follow the way conversions are done in the standard library.
  • All functions declared by the traits std::ascii::{AsciiExt, OwnedAsciiExt} carry the infix ascii which is redundant in the case the traits are implemented on Vec<Ascii>, [Ascii] and Ascii. This redundancy must be tolerated to achieve the goals described above.

Drawbacks

  • Duplicate implementations arise because the type Ascii implements the same functionality in the functions to_uppercase() / to_ascii_uppercase() and to_lowercase() and to_ascii_lowercase().

Further considerations after discussion

  • Deprecate and remove the functions to_uppercase() and to_lowercase() on Ascii in favour of their equivalents in AsciiExt. That removes the duplication mentioned in the drawbacks.
@SimonSapin
Copy link
Collaborator

Makes sense to me. There is no particular reason these impl don’t exist yet, I just didn’t think of adding them.

The traits std::ascii::{AsciiExt, OwnedAsciiExt} are marked experimental.

All stability markers were just copied from std::ascii without any thought and could use being revisited. Also it’s unclear that they mean anything or should be used outside of libstd. See discussion starting at rust-lang/rfcs#507 (comment)

Duplicate implementations arise because the type Ascii implements the same functionality

Just remove Ascii::to_uppercase and Ascii::to_lowercase.

@tomprogrammer
Copy link
Owner Author

See discussion starting at rust-lang/rfcs#507 (comment)

That is a good source of information, thanks. I consider rethinking the stability of the items in the crate.

Just remove Ascii::to_uppercase and Ascii::to_lowercase.

I didn't remove Ascii:to_uppercase and Ascii:to_lowercase because they are marked stable, but as long as rust has not reached 1.0 the API can change often. But the other stability markers also need recondsideration.

I'm also following the IO reform RFC in which the OsStrBuf, etc. types are also statically enforcing certain conformance. In that manner the motivation of OsStrBuf is similar to Ascii. You also participated there.

@SimonSapin
Copy link
Collaborator

I didn't remove Ascii:to_uppercase and Ascii:to_lowercase because they are marked stable

Again, the stability markers only have the meaning we give them. The point of moving things out of libstd is that you don’t have to provide the stability promises. It’s OK to make breaking/backward-incompatible changes as long as the version number is changed appropriately per SemVer. (Of course, as a library matures and gets more relied on, breaking changes should become infrequent and made more thoughtfully…)

@tomprogrammer
Copy link
Owner Author

I had a very strong reliance on the stability markers. The discussion you linked above brought some new points about the stability markers to me I didn't consider before. You are right that respecting SemVer is usually enough for a library to not break code using it immediately. Thanks for making this clear to me.

@SimonSapin
Copy link
Collaborator

It all depends on how many compatibility promises you, as a library maintainer, want to make. "Zero promises", although maybe painful for users, is a valid position. Stability markers and SemVer are just tools to express this. The Rust ecosystem has a relatively strong convention on SemVer, but you don’t have to use it either.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants