-
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(infra): share get_absolute_path infra util with snapi test utils #2075
Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. |
Artifacts upload triggered. View details here |
63ca81d
to
5e61b82
Compare
0141a56
to
42e510a
Compare
Artifacts upload triggered. View details here |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2075 +/- ##
===========================================
+ Coverage 40.10% 77.21% +37.11%
===========================================
Files 26 386 +360
Lines 1895 40386 +38491
Branches 1895 40386 +38491
===========================================
+ Hits 760 31185 +30425
- Misses 1100 6899 +5799
- Partials 35 2302 +2267 ☔ View full report in Codecov by Sentry. 🚨 Try these New Features:
|
42e510a
to
104ef4e
Compare
Artifacts upload triggered. View details here |
5e61b82
to
17634f4
Compare
104ef4e
to
cb4bbcb
Compare
Artifacts upload triggered. View details here |
17634f4
to
acaf6a4
Compare
cb4bbcb
to
f154aa3
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 14 files reviewed, 1 unresolved discussion (waiting on @ArniStarkware)
crates/infra_utils/src/path.rs
line 6 at r1 (raw file):
pub static PATH_TO_CARGO_MANIFEST_DIR: LazyLock<Option<PathBuf>> = LazyLock::new(|| env::var("CARGO_MANIFEST_DIR").ok().map(|dir| Path::new(&dir).into()));
Please make this a private function, and create a function that returns the cloned path. The benefit of that would be that
- users wouldn't have to clone the path whenever they use it
- we can later add code that returns this path even when running not through cargo
Code quote:
pub static PATH_TO_CARGO_MANIFEST_DIR: LazyLock<Option<PathBuf>> =
LazyLock::new(|| env::var("CARGO_MANIFEST_DIR").ok().map(|dir| Path::new(&dir).into()));
acaf6a4
to
b179f10
Compare
f154aa3
to
4ec2420
Compare
Artifacts upload triggered. View details here |
Benchmark movements: |
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 14 files reviewed, 1 unresolved discussion (waiting on @Itay-Tsabary-Starkware)
crates/infra_utils/src/path.rs
line 6 at r1 (raw file):
Previously, Itay-Tsabary-Starkware wrote…
Please make this a private function, and create a function that returns the cloned path. The benefit of that would be that
- users wouldn't have to clone the path whenever they use it
- we can later add code that returns this path even when running not through cargo
Done.
We can move functionality from get_absolute_path
into a function called: manifest_dir
, in another PR.
e9d42d0
to
eb4e92e
Compare
4ec2420
to
ca5efcc
Compare
Artifacts upload triggered. View details here |
eb4e92e
to
5719c03
Compare
ca5efcc
to
4686fa7
Compare
Artifacts upload triggered. View details here |
Benchmark movements: full_committer_flow performance regressed! |
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 6 of 14 files at r1, 8 of 8 files at r2, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @ArniStarkware)
4686fa7
to
a985e92
Compare
Artifacts upload triggered. View details here |
Artifacts upload triggered. View details here |
Benchmark movements: |
No description provided.