-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
Fix file comment #51988
Fix file comment #51988
Conversation
When you had a file containing the pattern to comment in both commented and uncommented form, the old logic was wrong. In such a case it was not commenting the lines that could be commented and instead returned that the pattern was already commented. The new logic gives commenting precedence: if at least one line exists that can be commented, they all will be, no matter if other lines that have already been commented exist. Only if no lines to comment exist will a check be made to see if there's an already commented pattern or if the pattern doesn't exist at all. The new behavior matches the file.comment module function's behavior. This fixes #41818.
When you had a file containing the pattern to comment in both commented and uncommented form, the old logic was wrong. In such a case it was not uncommenting the lines that could be uncommented and instead returned that the pattern was already uncommented. The new logic gives uncommenting precedence: if at least one line exists that can be uncommented, they all will be, no matter if other lines that have already been commented exist. Only if no lines to uncomment exist will a check be made to see if there's an already uncommented pattern or if the pattern doesn't exist at all. The new behavior matches the file.uncomment module function's behavior. See #41818 for an analog problem in the file.comment state function.
@mbunkus Looks like this is causing some test failures. |
I'll be out of town for several days. I'd appreciate it if someone else could fix up the tests. If not, I'll try to get around to it end of next week or so. |
I cannot run the tests at the moment. I've never done that before, so I don't know what the problem might be. I have a venv set up & activated; it's the one I use for running my changed states/modules. I did read
Yeah… there's no file or directory whose name contains
So… what now? |
Oh, |
I've fixed the unit test logic to accommodate the change to the |
[master] Porting #51988 to master
What does this PR do?
It fixes both the
file.comment
andfile.uncomment
state functions's behavior: both failed to do anything if the pattern is present in both commented and uncommented form.What issues does this PR fix or reference?
It fixes #41818.
Previous Behavior
When both commented and uncommented lines with the pattern existed, both states did nothing and reported that the pattern was already (un)commented. Lines that could be (un)commented were not.
New Behavior
The logic is reversed. If there's at least one line to (un)comment, all relevant lines will be (un)commented. Only if there's nothing to do will a search for an already (un)commented line be made in order to make the return message more meaningful (either "pattern already (un)commented" or "pattern not found").
Tests written?
No
Commits signed with GPG?
Yes