fix: respect pre-commit reformats when bumping #505
Conversation
Codecov Report
@@ Coverage Diff @@
## master #505 +/- ##
==========================================
+ Coverage 97.85% 97.99% +0.14%
==========================================
Files 39 39
Lines 1539 1548 +9
==========================================
+ Hits 1506 1517 +11
+ Misses 33 31 -2
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
|
Thanks! @Lee-W ? LGTM |
|
Automatically retrying something as potentially permanent as a commit seems quite risky considering Commitizen has no idea what caused the error. We also run Prettier and many other linters via pre-commit and have never had it automatically reformat anything generated by Commitizen. What types of changes is Prettier making? Indentation I'm guessing from your issue? I would suggest this feature be governed by an option (e.g., |
In reality there's no risk. If something fails because of reasons the 1st time (e.g.: disk is full, a linter fails...), it will fail with the same reasons the 2nd time. But if the reason is that a formatter changed the format of the CHANGELOG.md file, then doing the retry will be simply making cz compatible with any pre-commit toolkit out there. I maintain a similar patch in Copier and it's worked perfectly for years. I don't think we really need any option to implement this. AFAICS It's a sane fix that works out of the box without any side effects.
That's one of them. Another one is when a commit contains illegal (or imperfect) markdown in the title. For example, this commit produced a line like: - use _mrchef origin only privatelyand Prettier modified it to add a Both use cases are exercised in the supplied test. Just remove the patch and run the test, you'll see the failure. |
|
Consider what would happen if a tool (or even just a confused human) were attempting to parse the output though or something else (e.g., an alert regarding the error) was triggered an extra time unintentionally. It would be desirable to have the ability to configure Commitizen in a manner that would avoid alerting twice per error in such cases. Some commit failures are ephemeral, such as a pre-commit hook that conducts a security audit or otherwise hits the network for any reason whatsoever. A really frustrating situation could arise in the presence of a totally unrelated but buggy pre-commit hook that snuck in because it always worked when retried, but sometimes didn't work on the first try. I meant to say that |
|
If I understand you properly, you're worried that if, for example, somebody sets up a pre-commit hook that launches a rocket 🚀 every time they attempt to commit, we'd be sending two rockets 🚀 🚀 instead of one? That's very unlikely to happen. And if it happens, it's probably more a bug in the use of pre-commit than a bug in commitizen. I don't think it's a use case that should worry us. The user would be sending 2 rockets anyway whenever prettier reformats another file. Just think about this: if committing changes to CHANGELOG.md fails the 1st time, why would someone ever want to run OTOH, if committing changes to CHANGELOG.md fails the 2nd time, why would you ever want to run The point is: if your use case was working before this patch, it will continue working (1st commit attempt passes OK). And if it was failing, it could get fixed (if it was a reformat), or it could continue failing (if it was a real failure). So there's nothing we lose with this fix (unless you send rockets with commits, of course). 😁 I'll add some logs for the retry however. That seems an easy way to tell what's happening behind the scenes, in case it matters for some users. |
|
Logs added. In debug mode, you get full error. |
Not at all. I'm saying a "launch" button that fires two rockets instead of one violates the user design principle of least surprise even if it notifies the user it did so after the fact in the fine print (although that's clearly better than nothing).
I don't know that I follow the metaphor, but if they wrote code that handles a commit failure in some manner, it would only be triggered once unless they themselves committed twice. Prettier doesn't automatically retry commits. Everyone accepts that two rockets will fire if they click "launch" twice.
I am not following this, but the simplest reason someone would want to retry 0 times is that they have no reason to expect that retrying would ever be constructive (e.g., they aren't running auto-formatters as pre-commit hooks that may potentially clash with Commitizen). They may not want to spend time sorting through unexpected duplicate logs and/or time + money (e.g., build credits) re-running all of their other pre-commit hooks to no avail. They may not want an ephemeral error to be swallowed without even knowing what happened or that anything even had happened. And again, this retry logic can mask a bug in another hook that sometimes fails on the first attempt, but never on the second. On the other hand, they may not have any other pre-commit hooks, in which case retrying seems quite bizarre. There are a huge number of scenarios in computing where something can fail and be retried, and it's (a) quite rare for that retry behavior not to be configurable by the developer in polished APIs, and (b) not retrying is by-and-large the default, because it often results in more robust systems overall. |
|
Just try to think in one practical example where having this fix is worse than not having it. If you can't find any, it means that will just never happen. 🤷🏼♂️ Can't we just merge and fix that theoretical bug once your theories really happen somewhere? 🙄 |
|
In the most common case, retrying is not productive and may even be detrimental. |
|
I'll add the cli option.
|
Fix commitizen-tools#502. Just add `--retry` to `cz bump` and it will attempt to commit twice if the 1st commit fails. Useful if your 1st commit runs a formatter that can for example change the contents of CHANGELOG.md.
|
Option added. Now, if you don't run it with |
|
Thank you! |
|
So is there anything else left to merge? |
|
The feature seems to cover specific situation: To avoid confusions, it would be nice to document the feature 👍🏻 thanks! |
Includes an update to the `cz bump --help` output.
|
Documented. |
…ith or without --retry
Description
Fix #502. Just attempt to commit twice if the 1st commit fails.
Checklist
./script/formatand./script/testlocally to ensure this change passes linter check and testExpected behavior
Compatible with prettier.
Steps to Test This Pull Request
Changelog is now compatible with prettier + editorconfig.
Additional context