Closed brigand closed 5 years ago
Thanks for the pull request! :beers:
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 0.14
instead of develop
. So, to that end, I've updated 0.14
to bring it inline with all the changes made on develop lately (since I haven't been super actively working on 0.14
). You should be able to base it on 0.14
, but please let me know if you have any problems!
Afterward, I'll review the code in more depth. 😄
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), [...]
Per the IETF RFC 2812 grammar that #143 cites, the user
and host
components of prefix
es, while optional, may not be the empty string. I would suggest not having multiple ways to indicate that such a component is absent, but rather either
user
and host
components as String
rather than Option<String>
, and using the empty string as the sole means of indicating that a component was absent; orOption<S>
where S
is some string type that can't be the empty string.(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).
Thanks for the feedback!
@8573 I agree, None and the empty string are semantically the same here. Changed over to that, which also simplified the parser.
@aatxe rebased on 0.14, and it's ready for review.
@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?
You may have missed them because they're in irc-proto
, rather than irc
.
This distinction only exists on 0.14 right now.
(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 cargo test
locally.
Edit: had to cd
into irc-proto
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.