Open Cyclonit opened 7 months ago
The inconsistent behaviour in TextProcessor.run:228 makes coming up with a non-breaking and clean solution difficult. Thus I think we need to decide whether we are willing to do a breaking change. If we are, we could change replace_command
to replace each match like replace
does.
The switch between the old and new behaviour could be reintroduced by having an alternative match
to the current pattern
. If pattern
is used, the entire string is replaced. If match
is used, only the matches are replaced.
Let's not make a breaking change with this one. Can you share some config snippets / usage examples about how the new match
option will look like? I would say let's agree on something simple and document both behaviors.
definitely interested in this functionality as I would like to be able to switch out the repo url dynamically from our build system and would rather not have to call another script post changelog generation. I understand the desire to not to introduce a breaking change but surely that's the point of semver. Alternatively adding an additional parameter, or a different replace_
option makes it non breaking but allows introduction of the requested functionality?
And obv supporting env var / shell commands as replace_command
does would be key too.
I've done some more thinking on this since opening the issue, and I strongly believe that a breaking change would be the best cause of action. The inconsistent behaviour of replace
and replace_command
is difficult to reason for. Any attempts I have made to introduce additional parameters that would allow replacing just part of the changelog using external commands were unsuccessful.
The current behaviour of replace_command
is needed for use cases like running the entire changelog through external tools like typos
. This functionality could easily be achieved by changing the pattern used from .*
to (?s:.*)
. The flag s
allows .
to match newlines, thus making the pattern match the entire string.
@orhun, could you chime in on this? If a breaking changes is permissible, I'd give implementing it a shot.
I gave this another thought and I guess making a breaking change is tolerable for the sake of having a more consistent functionality. @Cyclonit sure, go ahead!
I don't really see a reason for an immediate breaking change if a simple additional field could solve the issue in a backward-compatible way:
postprocessors = [
{ pattern = '<REPO>', replace_command = 'echo "https://github.com/orhun/git-cliff"', local_replace = true }
]
or similar. AFAIU local_replace
would default to true
for replace
and to false
for replace_command
, which is obviously confusing, but it could be a temporary workaround and the breaking change could be added to a queue for a 3.0 release
I don't think a breaking change is necessary, and I get some millage out of the existing behavior. I think this is just a case of Naming Is Hard™ and a poorly named API. The existing replace
and replace_command
sound like they are related, when really one is a linewise filter and one is a search and replace. Maybe replace_command
should have been called pipe_line
or something. In any case it sounds like what you guys want is just another new mode, and either a new command that operates on the pattern match instead of lines where a match was made or a new flag changing the meaning of the current command sound like better options than rug-pulling the feature some of like the way it is now.
I think this is just a case of Naming Is Hard™ and a poorly named API.
Unfortunately yes 😔
Happy to move forward with this with the suggested non-breaking changes.
Is there an existing issue or pull request for this?
Feature description
Currently the behaviour of
replace
andreplace_command
are inconsistent.replace
will replace all matches ofpattern
with a given string, whereasreplace_command
replaces the entire string as long as at least one match was found.See TextProcessor.run:228 for references.
For example, the following
changelog.postprocessor
will replace the entire changelog instead of only the matched occurrences of<REPO>
.Desired solution
This issue is intended to serve as a place to discuss possible solutions.
Alternatives considered
None
Additional context
No response