-
Notifications
You must be signed in to change notification settings - Fork 25
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
chore: share the test util get_absolute_path #2026
Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. |
Artifacts upload triggered. View details here |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2026 +/- ##
===========================================
+ Coverage 40.10% 77.11% +37.01%
===========================================
Files 26 372 +346
Lines 1895 39702 +37807
Branches 1895 39702 +37807
===========================================
+ Hits 760 30618 +29858
- Misses 1100 6805 +5705
- Partials 35 2279 +2244 ☔ View full report in Codecov by Sentry. |
7d4ce6d
to
048f28d
Compare
9884a64
to
7f7233a
Compare
Artifacts upload triggered. View details here |
048f28d
to
32ea289
Compare
7f7233a
to
88a733d
Compare
Artifacts upload triggered. View details here |
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: 0 of 10 files reviewed, all discussions resolved (waiting on @Itay-Tsabary-Starkware)
crates/papyrus_config/Cargo.toml
line 26 at r1 (raw file):
lazy_static.workspace = true papyrus_test_utils.workspace = true starknet_api.workspace = true
@Itay-Tsabary-Starkware - Correct me if I am wrong, but you are now in charge of this crate
As discussed, starknet_api should be at the bottom of crate hierarchies. The config crate should also be low in the hierarchy, which might be problematic if we have an inheritance here.
Code quote:
starknet_api.workspace = true
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 10 of 10 files at r1, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @ArniStarkware)
crates/papyrus_config/Cargo.toml
line 26 at r1 (raw file):
Previously, ArniStarkware (Arnon Hod) wrote…
@Itay-Tsabary-Starkware - Correct me if I am wrong, but you are now in charge of this crate
As discussed, starknet_api should be at the bottom of crate hierarchies. The config crate should also be low in the hierarchy, which might be problematic if we have an inheritance here.
Right. IMO this is accetpable for the time being, but the more thought I put into this the more it seems we need this in a desginated util crate with this function as production code. You can see there are plenty of instances where the code opens files using relative paths in an unchecked manner.
88a733d
to
99c0f2f
Compare
32ea289
to
61ad4b1
Compare
99c0f2f
to
e127c69
Compare
Artifacts upload triggered. View details here |
Artifacts upload triggered. View details here |
Artifacts upload triggered. View details here |
Artifacts upload triggered. View details here |
Artifacts upload triggered. View details here |
Merge activity
|
No description provided.