Multiline support #98
Conversation
| @@ -275,7 +333,7 @@ def compute_change_request_for_file(self, file: tbump.config.File) -> ChangeRequ | |||
|
|
|||
| to_search = None | |||
| if file.search: | |||
| to_search = file.search.format(current_version=re.escape(current_version)) | |||
There was a problem hiding this comment.
This one looked like redundant to me, as well as blocked using the current_version later on in regex expressions correctly.
There was a problem hiding this comment.
I think you're right. Could you make a separate commit/PR so we can see if this is true?
| @@ -21,6 +21,10 @@ def test_file_bumper_simple(test_repo: Path) -> None: | |||
| assert file_contains(test_repo / "glob-one.c", 'version_one = "1.2.41-alpha-2"') | |||
| assert file_contains(test_repo / "glob-two.v", 'version_two = "1.2.41-alpha-2"') | |||
|
|
|||
| # one occurence is bumped up, others remain unchanged | |||
| assert file_contains(test_repo / "Cargo.toml", 'version = "1.2.41-alpha-2"') | |||
There was a problem hiding this comment.
To simplify my life, I did not created a separate test for checking multi-line replacement results. So I've re-used this one here to check that at least one line has been bumped, and at least one did not.
I'm aware this is not 100% accurate nor sufficiently strict assertion. It rather should be:
- exactly first occurrence of version string has been changed, and
- two other occurrences remain unchanged
Again, saving time here. Feel free to extend this check to more strict if you like.
There was a problem hiding this comment.
I think we really need a separate test. The goal of a test suite is to have exactly one test failing if there's a bug somewhere, so it would be better to have a dedicated test for the multiline case.
And of course, the test should check the entire contents of the replaced file :)
| new_line = old_line.replace(old_string, new_string) | ||
| patch = Patch( | ||
| self.working_path, str(expanded_src), i, old_line, new_line | ||
| # Checking for multi-line search phrase |
There was a problem hiding this comment.
I think we need a better architecture here. They are a lot of duplication between the two branches of the if "\\n" in search.
Ideally, we should just have something like his:
patcher = make_patcher(search)
patcher.patch()
where the make_patch function would be responsible for building either a MultilinePatch or a Path instance.
Also, there's some duplication between those two classes, which I think we could fix by implementing the following clas diagram:
... maybe without BasePatch inheriting from Action, but rather Action containing a BasePatch instance?
Anyway, feel free to contact me if you want to discuss this further.
There was a problem hiding this comment.
will come back to this PR when I can allocate some extra time.
|
Closing as it looks like not gonna land anytime soon. |
About
This is a PR covering feature idea from #83.
I've noticed some work started on
dm/multilinebranch, but I wasn't sure what the status of that was, (looked stalled to me).Using a bit of spare time, here is my proposed extension to support the feature.
Implementation is based on the idea that when line-ending characters are detected in search phrase, then separate
MultilinePatchclass handles the file patching. For that, a Python's builtinreregex utils withre.MULTILINEflag are used.The code for per-line search & replace like existed till now, is left unaffected, as I've been extending the code, rather modifying it.
All tests pass. The features used in extra test cases do correspond to what has been proposed in #83 description.
Implications
Since the patching for such case is file-based and not line-based (like the rest of the code), the UI information about the diff does not display numbers.