-
Notifications
You must be signed in to change notification settings - Fork 2
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
Parse box pattern syntax #95
Conversation
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.
Wonderful, thank you! Just some small tweaks, if you don't mind.
src/lib.rs
Outdated
@@ -2855,6 +2865,7 @@ shims! [ | |||
(asterisk, Token::into_asterisk, Error::ExpectedAsterisk), | |||
(at, Token::into_at, Error::ExpectedAt), | |||
(bang, Token::into_bang, Error::ExpectedBang), | |||
(box_pattern, Token::into_box, Error::ExpectedBox), |
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.
You can just reuse kw_box
, defined above (kw_
is to avoid conflict with the real keyword) :-)
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.
Ah dang, i knew it would be there but must have missed it! :D
src/lib.rs
Outdated
@@ -5337,6 +5361,12 @@ mod test { | |||
} | |||
|
|||
#[test] | |||
fn fn_with_arguments_with_box_pattern() { |
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 think it's safe to remove this test; we already cover that functions use the general set of "pattern" above. The presence of this test makes it look like there's something extra-special about box patterns in function arguments, which there isn't.
src/lib.rs
Outdated
@@ -1631,6 +1631,7 @@ pub enum PatternKind { | |||
String(PatternString), | |||
Struct(PatternStruct), | |||
Tuple(PatternTuple), | |||
Box(PatternBox) |
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.
Go ahead and place this alphabetically in the list, please.
3fdff39
to
0233796
Compare
Addressed your comments, should be fine now :) Thanks! |
Superb! Thanks again! If you don't mind me asking, how have you found using this library? Are you using it for anything exciting you want to share? |
Exams are over and i finally have time to rewrite explain-rs. (Rewrite because syntex is no longer maintained). Why i didn't choose syn / choose yours:
Your crate is mentioned though strata-rust several times in the issues of syn, mostly as a "this is how it should be done" example. |
Cool! As far as I know, I'm the only other user of this crate, so I'm probably blind to my own set of papercuts. Feel free to file an issue if you find any. |
Fixes #93