-
Notifications
You must be signed in to change notification settings - Fork 102
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
Introduces Prefix enum #149
Conversation
Thanks for the pull request! 🍻 Before I do a more in-depth review (mostly because I don't have a ton of time at this moment), let's knock out some higher-level things. #148 was merged already, so you should be able to rebase (and maybe have to do some git fiddling?) to get rid of 79e00bf. That being said, since this is a breaking change, it would be preferable to have it based on Afterward, I'll review the code in more depth. 😄 |
Per the IETF RFC 2812 grammar that #143 cites, the
(I suppose this is more a comment on the design in #143 than its implementation in these patches, but this ticket is where the action is.) A bit of searching on Crates.io doesn't show me any implementations of a non-empty string type, though, so I'd favor (1). |
2592115
to
f66dcd6
Compare
@8573 hehe, I'll cover my tracks by saying that I said "probably" 😉, but I agree that you've got the right idea. So, I'm glad @brigand followed. @brigand Thanks for the rebase! It looks like some tests are failing on Travis: test prefix::test::parse_danger_cases ...FAILED
test message::test::from_and_to_string ...ok
test message::test::new ...ok
test message::test::source_nickname ...FAILED
test message::test::to_message_invalid_format ...ok
failures:
---- prefix::test::parse_danger_cases stdout ----
thread 'prefix::test::parse_danger_cases' panicked at 'assertion failed: `(left == right)`
left: `"name!@"`
right: `"name"`', irc-proto/src/prefix.rs:115:9
note: Run with `RUST_BACKTRACE=1` for a backtrace.
---- message::test::source_nickname stdout ----
thread 'message::test::source_nickname' panicked at 'assertion failed: `(left == right)`
left: `Some("test@test")`
right: `Some("test")`', irc-proto/src/message.rs:313:9
failures:
message::test::source_nickname
prefix::test::parse_danger_cases Do these tests work on your machine? If not, can you fix them? |
(My apologies for taking issue with many things in your first Rust PR; don't be discouraged!) |
Oh, hmm, I'm not sure my tests are running for with Edit: had to Have to make some more fixes |
Should have mentioned, this is ready for review. |
Hi @brigand, Sorry for the delay. I've looked through everything, and I'm happy with it. This also adds another place where property testing would be useful (#135), but that can come later. Namely, the invariant you describe in the tests (round-tripping a string through Prefix should be lossless) could be tested with random testing. The code looks good to me! Cheers! 🍻 |
Hey, this is my first rust PR. Very open to making any changes.
Ref #143 Parsing message prefixes (sources) into an enumeration
There's an issue in that both of the following are allowed by the types, but there's only one string representation for both of them. The parser never produces this (gives the second), so it'd just be manual enum construction.
This is harmless until you try to do...
I'm not sure what this function should be named, but it seems weird doing
string.parse().unwrap()
everywhere, since it's not apparent at the call site that it never fails.Edit: not sure which branch this should be based on.