-
Notifications
You must be signed in to change notification settings - Fork 1
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
refactor: add parse_and_commit #309
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #309 +/- ##
==========================================
- Coverage 70.65% 70.51% -0.14%
==========================================
Files 38 38
Lines 2082 2086 +4
Branches 2082 2086 +4
==========================================
Hits 1471 1471
- Misses 541 545 +4
Partials 70 70 ☔ View full report in Codecov by Sentry. |
Benchmark movements: full_committer_flow performance improved 😺 |
Benchmark movements: full_committer_flow performance improved 😺 |
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.
Reviewed 3 of 3 files at r1, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @aner-starkware and @TzahiTaub)
crates/committer_cli/benches/committer_bench.rs
line 69 at r1 (raw file):
// let committer_input = parse_input(input_str).expect("Failed to parse the given input."); // commit(committer_input, OUTPUT_PATH.to_owned()).await; // }
delete
crates/committer_cli/src/main.rs
line 74 at r1 (raw file):
match args.command { Command::Commit { output_path } => { // TODO(Aner, 15/7/24): try moving read_from_stdin into function.
not sure this is really important; current method seems fine. your call
Code quote:
// TODO(Aner, 15/7/24): try moving read_from_stdin into function.
70dd03f
to
00cc8f8
Compare
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.
Reviewed 1 of 1 files at r2, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @aner-starkware and @TzahiTaub)
Benchmark movements: full_committer_flow performance improved 😺 |
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.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @aner-starkware and @TzahiTaub)
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.
Python side: https://reviewable.io/reviews/starkware-industries/starkware/35461#-
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @dorimedini-starkware and @TzahiTaub)
crates/committer_cli/src/main.rs
line 74 at r1 (raw file):
Previously, dorimedini-starkware wrote…
not sure this is really important; current method seems fine. your call
I'll look into it today\tomorrow; if I don't find a good solution, I'll just delete the TODO.
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.
Reviewable status:
complete! all files reviewed, all discussions resolved (waiting on @TzahiTaub)
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.
Reviewed 2 of 3 files at r1, 1 of 1 files at r2, all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @aner-starkware)
This change is![Reviewable](https://camo.githubusercontent.com/1541c4039185914e83657d3683ec25920c672c6c5c7ab4240ee7bff601adec0b/68747470733a2f2f72657669657761626c652e696f2f7265766965775f627574746f6e2e737667)