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

Add traits IntoAscii* to replace (Owned)AsciiCast #17

Merged
merged 4 commits into from
May 9, 2016

Conversation

tormol
Copy link
Collaborator

@tormol tormol commented Apr 6, 2016

AsciiCast has un-rustic names, and doesn't work well for Ascii (see first commit)

Another issue is that the traits aren't implemented for their target type,which breaks the pattern fn foo<B:Into<Bar>>(b: B).
Hopefully AsciiExt gets stabilized soon.
(Then this could be used by AsciiString.push())

IntoAsciiStr / The third commit is unfinished, just cherry-pick the first two if I don't finish it.

I haven't removed changed anything, so this is not a breaking change. The idea is to make a minor release, and then remove (Owned)AsciiCast, Ascii::from_byte() and more later.

@tormol
Copy link
Collaborator Author

tormol commented Apr 14, 2016

Why do you test on 1.1?

I split this and #18 into two pulls because I thought they were independent, But #18 would help in writing tests.
And some of the methods in Into(Mut)AsciiStr are complex enough to really need testing.

When you merge that I'll write those, rebase and squash some of the commits.

@tormol
Copy link
Collaborator Author

tormol commented Apr 19, 2016

Rebased and:

Prepended two semi-related commits because I didn't bother creating new pull requests.

In ascii_str:

  • Replaced Into with As in the conversion traits since they are basically failable As{Ref,Mut}.
    What do you think?
  • Added methods to the error struct.
  • Removed .ascii_part():
    Name didn't fit in with the other methods, and it can be replaced with s.into_ascii().map_err(|err| s[..err.index()].as_ascii().unwrap() ).
  • Wrote tests and fixed bug.

I thought about adding a method to AsAsciiStrError to return u8 or char, but there are enough types already. Most users will know whether they called .as_ascii() on an [u8] or str, but by making .char() return an option instead of panicing, generic converters can figure it out if they somehow want to.

@tomprogrammer
Copy link
Owner

This crate is tested on 1.1 to detect unintended breaking changes, because raising the compilers version is technically a breaking change. That said I don't think it isn't much of a problem to intentionally raise the version, if we test with that particular version. I will check breaking updates using the reverse dependencies feature crates.io provides.

I will cherry pick the commit "Implement Default, From<&mut[Ascii]> and As{Ref,Mut}<[Ascii]> for AsciiStr". Thanks!

I like the names AsAsciiStr and AsMutAsciiStr, they reflect the names of the infallible conversion traits in std::convert. I'm also fine with removing .ascii_part(), we instead may want to introduce lossy conversions, which are infallible. Lossy conversions are stuff for another PR since there arise some questions how to implement and document them to reduce confusion among users.

It's a bit unfortunate that even IntoAscii exists, maybe we can unify it with IntoAsciiString, since Ascii is inherently owned?

Thanks for your work and sorry for the delay I caused!

@tormol
Copy link
Collaborator Author

tormol commented May 4, 2016

Unifying traits would require an associated type, so <T: IntoAsciiString> becomes <T: IntoAscii<Target=AsciiString> which is quite a bit longer.
I can't see when one would want to be generic over Ascii and AsciiString either.

The library still builds on 1.1 (and travis still checks that), It's only the tests that doesn't.
As long as we or rust don't introduce silent breaking changes, shouldn't tests passing on later versions be valid for 1.1?
Must dev-dependencies follow semver?

In about a month AsciiExt will become available on stable. Then I think we should remove the unstable feature and make 1.9 the minimum. I have created a branch where I've started removing the old traits.

feature(ascii) has been stabilized on nightly.
In six weeks we can remove "unstable".
@tormol tormol force-pushed the into_ascii branch 2 times, most recently from 5248fa7 to b0c6b93 Compare May 8, 2016 00:51
tormol added 3 commits May 9, 2016 00:52
`AsciiCast` requires an explicit lifetime, which doesn't make sense for `Target=Ascii`:
`fn by_value<'a,C:AsciiCast<'a,Target=Ascii>>(ch: C) {…}` doesn't compile,
(because `'a` doesn't appear anywhere)
so you have to borrow ch: `fn by_ref<'a,C:AsciiCast<'a,Target=Ascii>>(c: &'a C) {…}`.
But then you have to explicitly borrow it in the caller: `by_ref(& 'a');` which looks bad.

The names are also more in line with what std uses: xxx_unchecked() and Into/From.
The only differences are trait and method names.
The error type says where and why conversion failed.
Two separate traits to follow std convention.

Stops testing on 1.1.0 because the tests doesn't compile there:

If we trust Rust to not introduce silent breaking changes,
code that passes tests on stable should be correct on older versions
as long as it compiles.
@tormol
Copy link
Collaborator Author

tormol commented May 8, 2016

Rebased and changed a few things:

  • Squashed the travis changes into Add AsAsciiStr.
  • Added some blank lines here and there.
  • Rewrote documentation.

@tomprogrammer
Copy link
Owner

Since these traits are mainly used for generic conversions I agree we should keep them separate. Four traits isn't too much, I'm simply over-engineering. ;)

I think skipping 1.1 for the tests is ok. What is the minimal required version for the tests? Maybe we want to increase the minimum rustc version that is guaranteed to work, advertising that earlier versions may work but are not tested. I have some changes in my queue that require rustc 1.3.

Mh, in my opinion dev-dependencies also must follow semver like normal dependencies do. The crate author doesn't know as which kind of dependency another programmer will use it.

I agree, great that AsciiExt is going to be stabilized.

This branch seems ready to merge, are you polishing the last bits and pieces or can I merge?

@tormol
Copy link
Collaborator Author

tormol commented May 9, 2016

I'm finished.

@tomprogrammer tomprogrammer merged commit c351e7e into tomprogrammer:master May 9, 2016
@tormol tormol deleted the into_ascii branch July 1, 2016 01:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants