-
-
Notifications
You must be signed in to change notification settings - Fork 197
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
Improve mailbox parsing using chumsky #839
Conversation
Thanks for all of the contributions you've been sending lately. |
I totally understand, that's why: please think well if you really want to add those changes to your lib and let me know. My 2 cents opinion: using parser combinators is losing a bit of performance for a more maintainable/readable/less buggy program, and I find it really efficient for parsing RFCs: you just need to go through sections, develop tiny parsers, test them, and then compose them together. If you do not want to add those changes directly inside your lib, I can also create a separated lib (in case you accept using chumsky). Think well and let me know! |
I think the performance penalty is worth it for for as you said maintainability and readability and I'd be happy to have it in lettre |
Awesome 🎉 I let you know when the PR is ready. |
I saw a test in the code base: #[test]
fn format_single_with_utf8_name() {
let from = vec!["Кайо <kayo@example.com>".parse().unwrap()];
let mut headers = Headers::new();
headers.set(From(from.into()));
assert_eq!(
headers.to_string(),
"From: =?utf-8?b?0JrQsNC50L4=?= <kayo@example.com>\r\n"
);
} and from the RFC the name part of a mailbox cannot contain non-ASCII chars, so parsing EDIT: I will go for the (2) and see how it behaves. |
I guess the PR is ready, I left some comments for you to get more context. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm glad to see the use of a proper parser builder. I haven't reviewed the actual parsing code closely but have some meta comments.
@kevincox thanks a lot for your review, I will reply whenever I can. I let you know. |
No rush :) |
@kevincox ready for another review whenever you can 🙂 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. This is looking really good. I think the only real blocking issue is how flexible we want to be with parsing mailboxes.
@kevincox @paolobarbolini any other changes in mind? If not then I guess it is ready for merge 🙂 |
Did you push the reduction of |
I'm preparing a 0.10.2 release at #853, after which we'll also make a 0.11 pre-release with this |
Not yet indeed, I will do those changes during this week.
Awesome 🙂 |
I removed parsers related to comments (I guess $ cargo bench --bench mailbox_parsing
Compiling lettre v0.10.1 (/home/soywod/code/lettre)
Finished bench [optimized] target(s) in 3.65s
Running benches/mailbox_parsing.rs (target/release/deps/mailbox_parsing-382c53ca32910006)
parse single mailbox time: [4.0927 µs 4.1017 µs 4.1130 µs]
change: [-81.903% -81.857% -81.812%] (p = 0.00 < 0.05)
Performance has improved.
Found 4 outliers among 100 measurements (4.00%)
1 (1.00%) low mild
2 (2.00%) high mild
1 (1.00%) high severe
parse multiple mailboxes
time: [11.352 µs 11.372 µs 11.390 µs]
change: [-76.088% -76.020% -75.958%] (p = 0.00 < 0.05)
Performance has improved.
Found 1 outliers among 100 measurements (1.00%)
1 (1.00%) high severe |
Ready for the last review! |
Ready for merge then 🎉 thanks a lot @kevincox for you help! |
I released 0.10.2. I'll give another look at the PR and then we can proceed. |
BTW I did |
Done, indeed the diff reduced and the CI seems to pass! |
Forget about it, plans changed! |
Any news about this? |
I'm about to release 0.10.3 just so I don't leave things sitting on master for too long and then together with #861 I'll merge this and either immediately or in the coming weeks release it as 0.11.0-alpha.1 |
Since #827 #832 and #838, I thought about improving the parsing using https://github.com/zesterer/chumsky. This lib helps to build solid parsers and to be as close as possible to what RFCs propose.
Here (in this draft PR) a preview of what it could look like, is it sth you would be interested in or not?