feat(JSON-RPC): support version without patch #929
Conversation
|
Current dependencies on/for this PR: This comment was auto-generated by Graphite. |
Codecov Report
@@ Coverage Diff @@
## main #929 +/- ##
==========================================
+ Coverage 74.34% 76.11% +1.76%
==========================================
Files 61 61
Lines 4977 5450 +473
Branches 4977 5450 +473
==========================================
+ Hits 3700 4148 +448
- Misses 569 600 +31
+ Partials 708 702 -6
... and 11 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
nagmo-starkware
force-pushed
the
support_version_without_patch
branch
from
dc02009
to
82e968e
Compare
There was a problem hiding this comment.
Reviewed 14 of 14 files at r1, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @nagmo-starkware)
crates/papyrus_gateway/src/gateway_test.rs line 122 at r1 (raw file):
#[tokio::test] async fn test_version_middleware() {
What should happen with a version with a lower patch than we have?
crates/papyrus_gateway/src/gateway_test.rs line 125 at r1 (raw file):
let base_uri = "http://localhost:8080/rpc/"; let mut path_options = vec![]; VERSION_CONFIG.iter().for_each(|(version_id, _)| {
Consider adding a version name with patch and lowercase v.
crates/papyrus_gateway/src/gateway_test.rs line 165 at r1 (raw file):
}; let mut rng = get_rng(); let version_id = VERSION_CONFIG.choose(&mut rng).unwrap().0;
Instead of first declaring the variable rng you can do:
let version_id = VERSION_CONFIG.choose(&mut get_rng()).unwrap().0;
crates/papyrus_gateway/src/middleware.rs line 101 at r1 (raw file):
return Err(BoxError::from(msg)); }; let version_comps = temp_version.split('_').collect::<Vec<_>>();
Consider doing the same thing by finding the first and last indexes of ''; if those equal no patch, else there is a patch. Then the version variable will be the String slice from the beginning to the first '' index.
Not very important...
nagmo-starkware
force-pushed
the
support_version_without_patch
branch
from
82e968e
to
8639748
Compare
There was a problem hiding this comment.
Reviewable status: 13 of 14 files reviewed, 3 unresolved discussions (waiting on @DvirYo-starkware)
crates/papyrus_gateway/src/gateway_test.rs line 122 at r1 (raw file):
Previously, DvirYo-starkware wrote…
What should happen with a version with a lower patch than we have?
the requirement we got from product says he'll get the latest version we have. we promise to provide you with the same minor version and a patch that is gte then what you asked for.
I guess I can add a test for that but I'm not sure if it's overkill. what do you think?
crates/papyrus_gateway/src/gateway_test.rs line 125 at r1 (raw file):
Previously, DvirYo-starkware wrote…
Consider adding a version name with patch and lowercase v.
I think it's unnecessary since if we see that the test supports both lowercase and capital v and also both with and without patch then we covered all the cases. can you think of a scenario it can break?
crates/papyrus_gateway/src/middleware.rs line 101 at r1 (raw file):
Previously, DvirYo-starkware wrote…
Consider doing the same thing by finding the first and last indexes of ''; if those equal no patch, else there is a patch. Then the version variable will be the String slice from the beginning to the first '' index.
Not very important...
did you mean _? if so then this wouldn't work since we have two underscores i.e. v0_3_0 and the last zero is the patch.
also that means iterating twice instead of once but it's less important since it's a short string anyway
There was a problem hiding this comment.
Reviewable status: 13 of 14 files reviewed, 3 unresolved discussions (waiting on @nagmo-starkware)
crates/papyrus_gateway/src/gateway_test.rs line 122 at r1 (raw file):
Previously, nagmo-starkware wrote…
the requirement we got from product says he'll get the latest version we have. we promise to provide you with the same minor version and a patch that is gte then what you asked for.
I guess I can add a test for that but I'm not sure if it's overkill. what do you think?
I think we should add a test for that.
crates/papyrus_gateway/src/gateway_test.rs line 125 at r1 (raw file):
Previously, nagmo-starkware wrote…
I think it's unnecessary since if we see that the test supports both lowercase and capital v and also both with and without patch then we covered all the cases. can you think of a scenario it can break?
It will be nice to know we cover all the cases here, and there are no actual costs for that (test runtime...).
crates/papyrus_gateway/src/middleware.rs line 101 at r1 (raw file):
Previously, nagmo-starkware wrote…
did you mean
_? if so then this wouldn't work since we have two underscores i.e.v0_3_0and the last zero is the patch.
also that means iterating twice instead of once but it's less important since it's a short string anyway
This is a minor optimization; you can forget it.
This is only one pass, to the first from the start and to the last from the end.
I don't understand the first point, but you can ignore this.
nagmo-starkware
force-pushed
the
support_version_without_patch
branch
from
8639748
to
a9eefa9
Compare
There was a problem hiding this comment.
Reviewable status: 13 of 14 files reviewed, 2 unresolved discussions (waiting on @DvirYo-starkware)
crates/papyrus_gateway/src/gateway_test.rs line 122 at r1 (raw file):
Previously, DvirYo-starkware wrote…
I think we should add a test for that.
added, but currently we don't have a case that checks this. look at the code and I think you'll understand
crates/papyrus_gateway/src/gateway_test.rs line 125 at r1 (raw file):
Previously, DvirYo-starkware wrote…
It will be nice to know we cover all the cases here, and there are no actual costs for that (test runtime...).
added it
DvirYo-starkware
force-pushed
the
support_version_without_patch
branch
2 times, most recently
from
0461f1d
to
82e968e
Compare
There was a problem hiding this comment.
Reviewable status:
complete! all files reviewed, all discussions resolved (waiting on @DvirYo-starkware)
Pull Request type
Please check the type of change your PR introduces:
What is the current behavior?
Issue Number: N/A
What is the new behavior?
Does this introduce a breaking change?
Other information
This change is