-
Notifications
You must be signed in to change notification settings - Fork 4
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
Use cable local checkout #255
Conversation
bc86387
to
c32237e
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #255 +/- ##
==========================================
+ Coverage 63.50% 63.84% +0.33%
==========================================
Files 35 35
Lines 2521 2597 +76
==========================================
+ Hits 1601 1658 +57
- Misses 920 939 +19 ☔ View full report in Codecov by Sentry. |
6dafe47
to
3f2f731
Compare
3f2f731
to
00078b1
Compare
436245b
to
6731ec0
Compare
c46accf
to
4218b87
Compare
4218b87
to
9e86846
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.
The implementation looks fine. But a few things to look into, especially in the tests.
Also, the documentation changes need to be added to this PR.
b910070
to
2981fd4
Compare
2981fd4
to
a9f9638
Compare
af0fc11
to
573adc9
Compare
573adc9
to
d26fa61
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.
I thought I only had small things to suggest but it turned into big things.
- We don't want to remove
rev_number-*.log
. These files were intended to allow to reproduce a previous run by recording the commit/revision numbers. The mechanism needs some review to be more useful, to be done later. But we shouldn't put in place anything that removes these files between realisations, otherwise they are completely useless. I have put in review suggestions which I think are enough to remove this but please check everything is gone. - I think we should phase out
internal.CWD
. It is a remnant from the start of the code and it isn't really needed now that we are using context managers to change directories. So can you rewrite the clean functions to usePath.cwd()
instead? And then the tests.
The rest of my comments are more minor stuff.
Co-authored-by: Claire Carouge <[email protected]>
5eb5b72
to
2bf9820
Compare
Co-authored-by: Claire Carouge <[email protected]>
2bf9820
to
4d31d37
Compare
benchcab/docs/user_guide/index.md Line 220 in 4d31d37
Here (see surrounding lines as well), it is suggested to remove Edit: the changes are done here in case if there's any change required benchcab/docs/user_guide/index.md Line 220 in d294866
|
Co-authored-by: Claire Carouge <[email protected]>
4d31d37
to
d294866
Compare
Co-authored-by: Claire Carouge <[email protected]>
d294866
to
512b9b8
Compare
Co-authored-by: Claire Carouge <[email protected]>
512b9b8
to
d06eb5f
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.
Two small changes. I approve as I think you can make these without me reviewing.
Co-authored-by: Claire Carouge <[email protected]>
d06eb5f
to
ac2cb1f
Compare
Resolves #33
Merge Message
benchcab clean
to remove directories created by benchcab, which cleanly removes symlinks. In case a user runsbenchcab
whensrc
already exists, the user would be shown a suggestion to runbenchcab clean
.benchcab clean
further options -submissions
deletes submission related files/dirs,realisations
deletes realisation related files/dirs,all
does both