-
Notifications
You must be signed in to change notification settings - Fork 31
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
Only show toolchain list once when using --installed
#88
Only show toolchain list once when using --installed
#88
Conversation
a855ca3
to
4777210
Compare
EDIT: see comment below from Joshua. I force pushed the fix to a test, it was panicking in the One thing I noticed is that Line 157 in a36602f
main can panic, in the other 9 places, the error is outputted and the program exits, maybe we should get rid of this one panic?
|
We cleared this up in DMs, this was a panic in the test suite, not cargo-sweep itself. |
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.
Thanks! Left a couple nits but this looks good overall :) could you please add a test showing the new behavior?
I think I'll wait for #78 to merge, to avoid giving him new conflicts again, and to reuse the project structure he created to test |
4b60471
to
59e0166
Compare
Changing to draft while we wait on #78 to be merged to reuse some of the testing code. |
c62fc85
to
332fc64
Compare
@@ -0,0 +1 @@ | |||
|
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 did this because it's simpler to run on tests/
, the alternative here would probably be to copy all projects to a tempdir
, create the target
folder in each copy, and run sweep -r -i
in this tempdir instead of tests/
.
Excuse me @jyn514, could you please update the label S-waiting-on-author to S-waiting-on-review? |
Sorry for the long delay. I am taking a semi-permanent break from github and don't know when I'll have time to maintain cargo-sweep. @marcospb19 you've done a lot of work on cargo-sweep in the past and I trust your judgement - are you interested in becoming a maintainer? :) |
In the meantime I'm happy to merge this as soon as you fix the conflicts. |
Have a great break! And thanks for everything you've done.
I feel flattered to have your trust! Yeah, I'm down! I believe the next good step would be to check out
Ok, I'll do that! |
332fc64
to
0cd0328
Compare
Rebase done, conflicts solved. |
Tests failing due to that bad regex on that specific test... let me see what I can do. Update to self: in |
the test `empty_project_output` used to expect Cargo to output exactly 4 files in the target folder, so we would delete them and see 4 lines as output, however, in MacOS we can see 5 files instead of 4 this change makes the test expect 2 or more files instead
Done by last commit! Clippy is failing due to a proc-macro2 update issue, which should be fixed by another PR. |
sounds perfect! check your email, you should have an invite :) |
Fixes #86.