-
Notifications
You must be signed in to change notification settings - Fork 893
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
Handle modifying rc files that don't have a trailing newline #2667
Conversation
I found the tests in the CI build, and added one for this. I ran |
I'm not sure why the test didn't run for you locally, you can clearly see it running in CI here - https://github.com/rust-lang/rustup/pull/2667/checks?check_run_id=1914524215#step:17:1345 |
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.
One potential tweak to be done to coments in the tests, and a break
which I think should be a continue
Otherwise this looks good, thank you for making the effort to contribute.
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.
This content is looking excellent, thank you for the extra test too, that is very useful.
Could you rebase the commits so that it's a clean change, and then we can look to merge this.
Thank you for your work.
651fa8e
to
4e56aea
Compare
Alright, rebased and squashed down to one commit. Thanks for the review and guidance! |
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'll merge on green-CI
This changes the flow so that instead of checking if
&rc.is_file
, it just tries to read the file and handles the error. I think this an ok way to handle it, though it might produce a little more IO. It simplifies the logic a lot.I couldn't find any tests that cover this function, and I'm not comfortable adding tests right now, but I could try and tackle that with some guidance, or if there is a similar test I could use an example.
Fixes #2636.