-
Notifications
You must be signed in to change notification settings - Fork 9
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
Implement parser for arithmetic expressions like a++
#155
base: main
Are you sure you want to change the base?
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.
Thanks a lot!
I also think cases such as echo $(++2)
are being parsed and created a part of the AST. I think we can raise error for this while creating the AST.
(parentheses_expr | VARIABLE | NUMBER) ~ post_arithmetic_op | ||
} | ||
|
||
|
||
unary_arithmetic_op = _{ |
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.
Can you change unary_arithmetic_op
to pre_arithmetic_op
for better clarity?
crates/deno_task_shell/src/parser.rs
Outdated
}) | ||
} | ||
Rule::unary_pre_arithmetic_expr => unary_pre_arithmetic_expr(first), | ||
Rule::unary_post_arithmetic_expr => unary_post_arithmetic_expr(first), | ||
_ => { |
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.
Shouldn't this result in an error given that unary_arithmetic_expr
can only be one of the unary_pre_arithmetic_expr
or unary_post_arithmetic_expr
?
crates/deno_task_shell/src/parser.rs
Outdated
|
||
fn unary_pre_arithmetic_expr(pair: Pair<Rule>) -> Result<ArithmeticPart> { | ||
let mut inner = pair.into_inner(); | ||
let first = inner.next().unwrap(); |
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 better to replace unwraps with proper error handling.
crates/deno_task_shell/src/parser.rs
Outdated
fn parse_unary_arithmetic_op_type(pair: Pair<Rule>) -> Result<bool> { | ||
match pair.as_rule() { | ||
Rule::unary_arithmetic_op => Ok(true), | ||
Rule::post_arithmetic_op => Ok(false), | ||
_ => Err(miette!( | ||
"Unexpected rule in unary arithmetic operator: {:?}", | ||
pair.as_rule() |
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.
This feels fairly hacky to return true of false for the type. Maybe you can try replacing this with match statement in unary_pre_arithmetic_expr
?
I believe in such examples, bash raises an error while compiling. I thnik it would be better to do the same here as well. |
a++
a++
@Armd04 Can you please take care of the issues on Windows? |
No description provided.