-
Notifications
You must be signed in to change notification settings - Fork 892
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 "true" for force_update in relevant install_from_dist calls in config.rs #2074
Use "true" for force_update in relevant install_from_dist calls in config.rs #2074
Conversation
af62fcd
to
6d26dfa
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 is good; but are you certain that the tests fail if you don't have the src/config.rs change?
@kinnison, I've done some local testing using latest Below seems to validate that Please let me know if there are other ways you'd like me to test to validate more. Thanks. `master` version failed the download
PR version installed
|
Right, but do your test cases behave this way -- an easy way to check that is to simply temporarily change the trues back to falses and run those new tests -- if they fail with |
6d26dfa
to
71d1e72
Compare
@kinnison, you were right about that the previous tests were inefficient that there was no difference when reverting the booleans. So I rewrote the test w. I also confirmed that test would fail before the boolean change, and then with the boolean change the test passed as expected. PTAL? Test failed before the boolean change in config.rs
Test passed after the boolean change in config.rs
|
71d1e72
to
f11e7f1
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.
If you can fix the test case, I think this looks good. Nice job.
Since I had some time, I made the change for you and pushed a commit. if you're okay with it, and it passes tests, then please fold it into your commit appropriately and then we'll be good to merge. |
09a24c5
to
0800f41
Compare
@kinnison, ah my bad. Didn't use wildcard w. P.S.: Thanks for your comment that led to the learning about |
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.
LGTM
This PR resolves #2068 by updating the
force_update
param fromtofalse
true
in relevantinstall_from_dist()
calls in config.rs, and provides two tests to mimic the users cases where theseinstall_from_dist
calls would occur. (Could update the tests better under review guidance. Thanks in advance.)