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
Repair automated commenting out of pre-4.0 sct settings #3805
Conversation
I fear this will be difficult to test, because I've never seen a pre-4.0 SCT-installed bashrc file. But I'll ask https://forum.spinalcordmri.org/t/sct-5-6-installation-failed/884/ to provide one, so hopefully that'll give us a check. |
regexes!! |
Codecov Report
Flags with carried forward coverage won't be shown. Click here to find out more. |
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.
Pre-approving, as I think my proposed change is relatively simple and easy to verify?
Co-authored-by: Joshua Newton <joshuacwnewton@gmail.com> Co-authored-by: Joshua Newton <joshuacwnewton@gmail.com>
Great, thanks for testing quickly!
I think your proposed patch isn't idempotent. The previous sed line was idempotent: it would replace 0 or more '#'s at the start of a line with a single '#'. Anyway you don't need to match the trailing line to comment it out. But I'm on my way out to the lab lunch so I'll won't be able to actually double check myself for an hour or two. Just try running it a couple times on the same file and see what it does.
|
@kousu Did something glitch here with your email response? It seems like this was intended to be sent some time ago... (I notice you sent a similar comment in #3805 (comment), so I presume this has been resolved already, especially given that this fix was later tweaked in #3872.) |
I was 😴 six hours ago. This isn't the first time GitHub has lagged email to/from me. |
But that's literally from months ago. Has it been sitting in some redis queue all this time? 🔄 |
Checklist
GitHub
PR contents
Description
See #3804
This was inadvertently broken in #3591.
Linked issues
Fixes #3804