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

feat(commit): add retry_after_failure config option and --no-retry flag #1027

Merged
merged 12 commits into from
Mar 30, 2024
Merged

feat(commit): add retry_after_failure config option and --no-retry flag #1027

merged 12 commits into from
Mar 30, 2024

Conversation

crai0
Copy link
Contributor

@crai0 crai0 commented Mar 15, 2024

Description

This pull request resolves #732 by implementing the requested feature.

Checklist

  • Add test cases to all the changes you introduce
  • Run ./scripts/format and ./scripts/test locally to ensure this change passes linter check and test
  • Test the changes on the local machine manually
  • Update the documentation for the changes

Expected behavior

See #732 (comment).

Steps to Test This Pull Request

  1. Set up a git repository with a git hook that always fails
git init test
cd test
echo '#!/usr/bin/env bash\necho "pre-commit fail"\nfalse' > .git/hooks/pre-commit 
chmod +x .git/hooks/pre-commit
  1. Enable retry_after_failure
echo '[tool.commitizen]\nretry_after_failure=true' > .cz.toml
  1. Try to create a commit (should fail)
git add .
cz commit
  1. Retry (still fails but reuses the previous message)
cz commit
  1. Force new message prompt (still fails but uses new message)
cz commit --no-retry

Additional context

Copy link

codecov bot commented Mar 15, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 97.48%. Comparing base (120d514) to head (888606b).
Report is 231 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1027      +/-   ##
==========================================
+ Coverage   97.33%   97.48%   +0.15%     
==========================================
  Files          42       55      +13     
  Lines        2104     2429     +325     
==========================================
+ Hits         2048     2368     +320     
- Misses         56       61       +5     
Flag Coverage Δ
unittests 97.48% <100.00%> (+0.15%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@noirbizarre noirbizarre left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great ! Thanks (I was going to take a loook at this, so perfect timing 👌🏼)

@noirbizarre noirbizarre added pr-status: ready-to-merge almost ready to merge. just keep it for a few days for others to check and removed pr-status: wait-for-review labels Mar 19, 2024

def read_backup_message(self) -> str:
def read_backup_message(self) -> Optional[str]:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
def read_backup_message(self) -> Optional[str]:
def read_backup_message(self) -> str | None:

@@ -1,12 +1,14 @@
import contextlib
import os
import tempfile

from typing import Optional
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
from typing import Optional

Comment on lines 7 to 8
print("could not import commitizen:")
print(error)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
print("could not import commitizen:")
print(error)
print(f"could not import commitizen:\n{error}")

Comment on lines 31 to 37
return os.path.join(
tempfile.gettempdir(),
"cz.commit%{user}%{project}.backup".format(
user=os.environ.get("USER", ""),
project=project,
),
)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
return os.path.join(
tempfile.gettempdir(),
"cz.commit%{user}%{project}.backup".format(
user=os.environ.get("USER", ""),
project=project,
),
)
user = os.environ.get("USER", "")
return os.path.join(
tempfile.gettempdir(),
f"cz.commit%{user}%{project}.backup"
)

@Lee-W
Copy link
Member

Lee-W commented Mar 30, 2024

Thanks so much for finishing this feature! I just fixed a few style-related nitpicks. Will merge this one once CI passes.

@Lee-W Lee-W merged commit b42a676 into commitizen-tools:master Mar 30, 2024
18 checks passed
@Lee-W
Copy link
Member

Lee-W commented Mar 30, 2024

Closes: #732

@crai0
Copy link
Contributor Author

crai0 commented Mar 30, 2024

@Lee-W awesome, thanks a lot.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr-status: ready-to-merge almost ready to merge. just keep it for a few days for others to check pr-status: wait-for-review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

New retry_after_failure config option and --no-retry overwrite flag
4 participants