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.

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

Add parameter to search & replace strictly #132

Closed
wants to merge 1 commit into from

Conversation

delbertooo
Copy link

@delbertooo delbertooo commented Feb 26, 2020

The current behavior of search and replace is:

If the pattern of search isn't found, try to search for the default version representation (not the pattern).

This may lead to unwanted or strange replacements. As I debugged such unwanted replacements I found a TODO in your code and decided to implement it. The new behavior (with the option enabled) will be:

If the pattern of search isn't found, there will be an error. The error will be the same error as usual if the version isn't found at all.

The implementation is opt-in and should not alter the current behavior of the program at all. If you don't specify the new option, everything will work as it did before.

I suggest bringing this into Version 1.1.0 if you follow semver.

- Add argument --search-strict and option search_strict (boolean)
- Add tests
@delbertooo
Copy link
Author

You might want to mention issues, if there are issues affected by this.

@florisla
Copy link
Collaborator

Hi delbertooo, did you see #128 ?

Do you think #128 covers your desired behavior, or is your implementation even more strict?

@delbertooo
Copy link
Author

delbertooo commented Feb 27, 2020

Hi, I - in fact - didn't see #128 and #127.

I strongly agree with #127 and going with this would make my feature obsolete. I think I got misled by the fact, that the code of this "fallback feature" is still there but dead now. The side effect, that the existence of the version is checked before replacing makes this code dead (when called from CLI).

TL;DR: I think my code isn't needed if everyone agrees with #127.

Besides that: as you reached the first major version, I would strongly suggest to follow semantic versioning. Introducing #127 and #128 is a breaking change to existing bumpversion derivates and to bump2version itself. I don't think we can declare #127 as a bug, even though it's a pretty useless feature. So IMO this should be part of a new major version with migration notes in a corresponding UPGRADING.md file. But this topic isn't related to the original issue anymore.

@delbertooo delbertooo closed this Jun 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants