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

chore: share the test util get_absolute_path #2026

Merged
merged 1 commit into from
Nov 14, 2024

Conversation

ArniStarkware
Copy link
Contributor

No description provided.

@reviewable-StarkWare
Copy link

This change is Reviewable

Copy link

Artifacts upload triggered. View details here

@ArniStarkware ArniStarkware marked this pull request as ready for review November 13, 2024 14:34
Copy link

codecov bot commented Nov 13, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 77.11%. Comparing base (e3165c4) to head (e127c69).
Report is 383 commits behind head on main.

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.
📢 Have feedback on the report? Share it here.

@ArniStarkware ArniStarkware force-pushed the arni/snapi/test_utils/move_nonce_manager_near_the_impl branch from 7d4ce6d to 048f28d Compare November 14, 2024 09:00
@ArniStarkware ArniStarkware force-pushed the arni/test_utils/share_get_absolute_path branch from 9884a64 to 7f7233a Compare November 14, 2024 09:00
Copy link

Artifacts upload triggered. View details here

@ArniStarkware ArniStarkware force-pushed the arni/snapi/test_utils/move_nonce_manager_near_the_impl branch from 048f28d to 32ea289 Compare November 14, 2024 09:51
@ArniStarkware ArniStarkware force-pushed the arni/test_utils/share_get_absolute_path branch from 7f7233a to 88a733d Compare November 14, 2024 09:51
Copy link

Artifacts upload triggered. View details here

Copy link
Contributor Author

@ArniStarkware ArniStarkware left a 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

Copy link
Contributor

@Itay-Tsabary-Starkware Itay-Tsabary-Starkware left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 10 of 10 files at r1, all commit messages.
Reviewable status: :shipit: 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.

@ArniStarkware ArniStarkware changed the base branch from arni/snapi/test_utils/move_nonce_manager_near_the_impl to graphite-base/2026 November 14, 2024 10:23
@ArniStarkware ArniStarkware force-pushed the arni/test_utils/share_get_absolute_path branch from 88a733d to 99c0f2f Compare November 14, 2024 10:23
@ArniStarkware ArniStarkware changed the base branch from graphite-base/2026 to main November 14, 2024 10:24
@ArniStarkware ArniStarkware force-pushed the arni/test_utils/share_get_absolute_path branch from 99c0f2f to e127c69 Compare November 14, 2024 10:24
Copy link

Artifacts upload triggered. View details here

Copy link

Artifacts upload triggered. View details here

Copy link

Artifacts upload triggered. View details here

Copy link

Artifacts upload triggered. View details here

Copy link

Artifacts upload triggered. View details here

@ArniStarkware ArniStarkware merged commit 604ce23 into main Nov 14, 2024
22 checks passed
Copy link
Contributor Author

Merge activity

  • Nov 14, 5:47 AM EST: A user merged this pull request with Graphite.

@github-actions github-actions bot locked and limited conversation to collaborators Nov 16, 2024
@ArniStarkware ArniStarkware deleted the arni/test_utils/share_get_absolute_path branch November 28, 2024 13:46
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants