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

Clean up trait implementations. #33

Merged
merged 1 commit into from Nov 22, 2018

Conversation

Screwtapello
Copy link
Collaborator

Now every struct has the complete set of AsRef, Borrow and From impls to get at the things inside it. There are also From impls for constructing structs where that is infallible.

Into impls have been removed, since the standard library has a blanket impl for everything that implements From.

Now every struct has the complete set of AsRef, Borrow and From impls to get at
the things inside it. There are also From impls for constructing structs where
that is infallible.

Into impls have been removed, since the standard library has a blanket impl for
everything that implements From.
@vitiral
Copy link
Owner

vitiral commented Nov 20, 2018

Hey, sorry I haven't gotten back to you on this yet. I started to look at it and it looks good, but haven't completed review.

@vitiral
Copy link
Owner

vitiral commented Nov 20, 2018

If it really is just a "clean up", i.e. does not break any API, feel free to merge though.

@Screwtapello
Copy link
Collaborator Author

I'm... pretty sure it's just a cleanup, but implementing a trait is considered a semver-minor change, so it'll be good to have another pair of eyes over it.

Copy link
Owner

@vitiral vitiral left a comment

Choose a reason for hiding this comment

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

Lots of boilerplate! Thanks for making things more complete

impl Into<PathArc> for PathAbs {
fn into(self) -> PathArc {
self.0
impl From<PathAbs> for PathArc {
Copy link
Owner

Choose a reason for hiding this comment

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

@@ -460,49 +460,30 @@ impl Deref for PathDir {
}
}

impl Into<PathAbs> for PathDir {
/// Downgrades the `PathDir` into a `PathAbs`
Copy link
Owner

Choose a reason for hiding this comment

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

Do we want to keep the docs? Probably not...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I considered it, but all these traits have pretty solid, unambiguous definitions, and have their own documentation which we don't really need to duplicate. The more docs we have, the more docs we need to maintain, so we should concentrate on the structs and functions that are unique to this crate instead of things from the standard library.

@vitiral
Copy link
Owner

vitiral commented Nov 21, 2018

I did a quick skim and checked to make sure it makes sense to implement these. Let me know if/when you want to do a release.

@Screwtapello
Copy link
Collaborator Author

I've still got my "replace PathArc with traits" change queued up; maybe you want to wait for that before doing a release, maybe you'd rather do one last release before it lands so people can make use of these changes without having to tackle all of them.

@Screwtapello Screwtapello merged commit 6b19eab into vitiral:master Nov 22, 2018
@Screwtapello Screwtapello deleted the trait-cleanup branch January 18, 2019 10:32
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.

None yet

2 participants