Skip to content
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

feat Add parsing of arithmetics #147

Merged
merged 7 commits into from
Sep 19, 2024
Merged

feat Add parsing of arithmetics #147

merged 7 commits into from
Sep 19, 2024

Conversation

prsabahrami
Copy link
Collaborator

@prsabahrami prsabahrami commented Sep 18, 2024

@prsabahrami prsabahrami changed the title Add parsing of arithmetics WIP Add parsing of arithmetics Sep 18, 2024
Copy link

codecov bot commented Sep 18, 2024

Codecov Report

Attention: Patch coverage is 35.88342% with 352 lines in your changes missing coverage. Please review.

Project coverage is 58.06%. Comparing base (144a8de) to head (49987a7).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
crates/deno_task_shell/src/shell/types.rs 26.10% 201 Missing ⚠️
crates/deno_task_shell/src/shell/execute.rs 42.55% 81 Missing ⚠️
crates/deno_task_shell/src/parser.rs 49.62% 67 Missing ⚠️
crates/deno_task_shell/src/shell/command.rs 0.00% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #147      +/-   ##
==========================================
- Coverage   63.55%   58.06%   -5.49%     
==========================================
  Files          28       28              
  Lines        2261     2795     +534     
==========================================
+ Hits         1437     1623     +186     
- Misses        824     1172     +348     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@prsabahrami
Copy link
Collaborator Author

@wolfv @certik Let me know how you think about the way AST is being created here. Then I can proceed with the execution.

@wolfv
Copy link
Member

wolfv commented Sep 18, 2024

To me, this looks great!

@prsabahrami
Copy link
Collaborator Author

Great! Then I'll proceed with execution and take care of the unwraps.

@prsabahrami
Copy link
Collaborator Author

This seems to be working. I have added support for almost everything except ++ and -- operators. Those are also easy to implement. Will take care of them afterwards.

@prsabahrami prsabahrami requested a review from wolfv September 19, 2024 06:32
@prsabahrami prsabahrami marked this pull request as ready for review September 19, 2024 06:32
Copy link
Contributor

@Hofer-Julian Hofer-Julian left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few minor comments

crates/deno_task_shell/Cargo.toml Show resolved Hide resolved
cmd.args,
&context.state,
context.stdin.clone(),
context.stderr.clone(),
)
.await
.map_err(|e| miette!(e.to_string()))
.map_err(|e| miette!(e.to_string()))?;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
.map_err(|e| miette!(e.to_string()))?;
.into_diagnostics()?;

Does this work as well?

@wolfv
Copy link
Member

wolfv commented Sep 19, 2024

I found like one little thing when testing.

A=5
echo $((1+A))
# doesn't work
echo $((1+$A))
echo $((1+${A}))

Not sure how hard that is. Not super important IMO!

@prsabahrami
Copy link
Collaborator Author

I found like one little thing when testing.


A=5

echo $((1+A))

# doesn't work

echo $((1+$A))

echo $((1+${A}))

Not sure how hard that is. Not super important IMO!

Thanks for pointing this out. If the behaviour in such cases is going to be the same as the first working example, then handling these would a slight modification to the parser if I'm not mistaken.

@wolfv
Copy link
Member

wolfv commented Sep 19, 2024

yep, definitely the same. Although with the ${A:1:2} syntax you could do additional things in the future (cf #148).

And then in the bash case, things like $((${A}1+1)) would also work but are probably annoying to support.

╭─wolfv@pixi ~/Programs/shell ‹support_arithmetic●›
╰─$ echo $A                                                                                                                                                                                                                                             
5
╭─wolfv@pixi ~/Programs/shell ‹support_arithmetic●›
╰─$ echo $((${A}5+1))
56

@prsabahrami
Copy link
Collaborator Author

Cool. I'll look into those and add support for. But, I also think we can add support for those after #148 is merged so that we'd take care of everything altogether. Let me know what you think!
Also, in regards to the bash examples, if the separator is { or }, it shouldn't be that hard. We can look into those later since I don't think they are in priority.

@prsabahrami prsabahrami changed the title WIP Add parsing of arithmetics feat Add parsing of arithmetics Sep 19, 2024
@wolfv wolfv merged commit 8f14fb5 into main Sep 19, 2024
5 of 7 checks passed
@wolfv wolfv deleted the support_arithmetic branch September 19, 2024 17:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants