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

Derive std traits for Target. #18

Merged
merged 2 commits into from Sep 1, 2017

Conversation

Projects
None yet
3 participants
@mjkillough
Copy link
Contributor

mjkillough commented Aug 22, 2017

Fixes #6.

@KodrAus
Copy link
Collaborator

KodrAus left a comment

Thanks @mjkillough! I've just left a simple comment.

src/lib.rs Outdated
@@ -157,7 +157,7 @@ mod filter;
mod filter;

/// Log target, either stdout or stderr.
#[derive(Debug)]
#[derive(Clone, Copy, Debug, Eq, Hash, Ord, PartialEq, PartialOrd)]

This comment has been minimized.

@KodrAus

KodrAus Sep 1, 2017

Collaborator

Hmm, Ord and PartialOrd probably don't make sense for this enum. I think we should remove them, but the rest look good.

This comment has been minimized.

@mjkillough

mjkillough Sep 1, 2017

Author Contributor

Sorry, I'd missed your comment on #6. I agree: I don't think they make sense, so I've removed them in the latest commit.

@mjkillough mjkillough referenced this pull request Sep 1, 2017

Closed

Implement missing imps for Target #6

0 of 7 tasks complete
@mjkillough

This comment has been minimized.

Copy link
Contributor Author

mjkillough commented Sep 1, 2017

@KodrAus - Thanks a lot for the review! Hopefully the latest commit addresses your comment. Please can you take another look?

@KodrAus

KodrAus approved these changes Sep 1, 2017

Copy link
Collaborator

KodrAus left a comment

This looks good to me now! Thanks.

@sebasmagri Are you happy for me to merge this in?

@sebasmagri

This comment has been minimized.

Copy link
Owner

sebasmagri commented Sep 1, 2017

Sure @KodrAus! Thanks!

@KodrAus KodrAus merged commit 5451f90 into sebasmagri:master Sep 1, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.