-
Notifications
You must be signed in to change notification settings - Fork 27
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
Merge smart contract test cargo files #1059
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #1059 +/- ##
==========================================
+ Coverage 60.49% 62.45% +1.95%
==========================================
Files 160 160
Lines 18590 18590
==========================================
+ Hits 11246 11610 +364
+ Misses 6240 5861 -379
- Partials 1104 1119 +15
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
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 31 of 31 files at r1, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @miladz68 and @ysv)
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 @masihyeganeh and @ysv)
a discussion (no related file):
The point of this fix was to remove all cargo file except one, but here we have added one more cargo file to be maintained. this was not the intention.
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 @miladz68 and @ysv)
a discussion (no related file):
Previously, miladz68 (milad) wrote…
The point of this fix was to remove all cargo file except one, but here we have added one more cargo file to be maintained. this was not the intention.
Yeah, this was my question to see if this adds anything to us or not. We can have one Cargo.toml
file that produces many binaries, but if we are going to produce libraries, each Cargo.toml
should be defined for each one.
The pros of this change is:
- having the actual dependencies in 1 file (others just point to the workspace)
- the dependencies are downloaded and cached only once for all of the projects and the target folder will be reused, so less compile time and less hard disk space, …
- all the
Cargo.lock
files are gone and only have 1 instead - some common part of the
Cargo.toml
files moved to the parent one, so they are all shorter
The cons of this change is:
- the
Cargo.toml
files are not removed which seems pointless in this case
Let's discuss to keep these changes or revert them
52f3dde
to
3364581
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 @miladz68 and @ysv)
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 30 of 31 files at r1, 1 of 1 files at r2, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @ysv)
Description
Using cargo workspace to merge all the
Cargo.toml
files and get rid ofCargo.lock
files and list dependencies in the workspace insteadReviewers checklist:
Authors checklist
This change is