Skip to content
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.

Sign up for GitHub

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

Jump to bottom

search-replace falls back to plain version lookup in some cases #198

Open
sanderr opened this issue Feb 17, 2021 · 3 comments · May be fixed by #262
Open

search-replace falls back to plain version lookup in some cases #198

sanderr opened this issue Feb 17, 2021 · 3 comments · May be fixed by #262
Labels
bug

Comments

@sanderr
Copy link

sanderr commented Feb 17, 2021

bumpversion.utils.contains is used as a guard to ensure that when the search pattern is not found, bump2version will not fall back to the default (as mentioned in the README). However, due to inconsistent matching behavior between bumpversion.utils.contains and bumpversion.utils.replace, in some cases it does fall back.

An example:
Suppose version.txt contains the following (note the leading spaces):

  name = my_package
  version = 1.0.0
  name = other_package
  version = 1.0.0

An attempt at a bumpversion.cfg file to match this:

[bumpversion]
current_version = 1.0.0

[bumpversion:file:version.txt]
search = name = my_package
  version = {current_version}
replace = name = my_package
  version = {new_version}
  • Expected behavior: either this bumps the first 1.0.0 and leaves the other one alone or this fails with an error message.
  • Actual behavior: it replaces both occurrences of 1.0.0 with the entire replace string, resulting in the following:
  name = my_package
  version = name = my_package
version = 1.1.0
  name = other_package
  version = name = my_package
version = 1.1.0

The cause is that bumpversion.utils.contains uses in to check whether the first and last lines match, while bumpversion.utils.replace uses str.replace. The former matches even if the last line has leading characters not in the search line or if the first line has trailing characters not in the search line. I believe this is incorrect. This method should use == lookbehind[0].lstrip() and == lookbehind[-1].rstrip() instead of in lookbehind[0] and similar.
@florisla florisla added the bug label Feb 18, 2021
@florisla
Copy link
Collaborator

florisla commented Feb 18, 2021 •
edited
Loading

I agree, the use of in is incorrect.

Not sure if we can use == lookbehind[0].lstrip() because that would mean that extra first-line leading characters (and last-line trailing characters) are not allowed in the target file. That's a choice we have to make.

The alternative is to use lookbehind[0].endswith(search_lines[0]). Where we perhaps need to right-strip both strings.

@sanderr sanderr mentioned this issue Feb 18, 2021
Open
@sanderr
Copy link
Author

sanderr commented Feb 18, 2021

You're right, lstrip and rstrip only strip whitespace of course, I agree lookbehind[0].endswith(search_lines[0]) would be better. I don't think you'd benefit from right-stripping both strings though (for the first line specifically) as it would introduce similar inconsistencies when compared to the behavior of str.replace. If there is trailing whitespace in either the search string or the file, it should be in the other as well to produce an exact match (though I don't think you should ever intentionally have trailing whitespace anywhere).

Come to think of it, have you considered just using search in f.read()? As far as I see this would be equivalent to the behavior we're talking about here, and it would certainly be consistent with str.replace. You'd only lose some logging information.

@sanderr
Copy link
Author

sanderr commented Feb 18, 2021

Additionally, if you'd use endswith, you'd have to consider the case where there's only one search line separately. If there's only one llne, in is correct I think.

@sanderr sanderr linked a pull request Oct 26, 2022 that will close this issue
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Issue/198 inconsistent search replace sanderr/bump2version
2 participants
@florisla @sanderr