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

Feature/allow empty commit #592

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

Dranaxel
Copy link

@Dranaxel Dranaxel commented Sep 18, 2022

Description

Add allow to create empty commit

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
    • One error on create commit message seem to be here before
  • Test the changes on the local machine manually
  • Update the documentation for the changes
    • Not needed

Expected behavior

Allow to invoke cz and create a commit without any change in it

Additional context

Relates to #247 & #590

@codecov
Copy link

codecov bot commented Sep 18, 2022

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (92b1e89) 98.32% compared to head (a4c9a1e) 98.33%.
Report is 401 commits behind head on master.

❗ Current head a4c9a1e differs from pull request most recent head 4f9c725. Consider uploading reports for the commit 4f9c725 to get more accurate results

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #592   +/-   ##
=======================================
  Coverage   98.32%   98.33%           
=======================================
  Files          39       39           
  Lines        1614     1618    +4     
=======================================
+ Hits         1587     1591    +4     
  Misses         27       27           
Flag Coverage Δ
unittests 98.33% <100.00%> (+<0.01%) ⬆️

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.

@Dranaxel
Copy link
Author

@Lee-W Can you elaborate about what bother you in this test case ?

@Lee-W
Copy link
Member

Lee-W commented Sep 22, 2022

I'm kinda not sure whether this is correctly tested and will need sometime to figure it out. Will try to check it this week

@Dranaxel
Copy link
Author

Hi, I think we have a misunderstanding here. When I say "Empty commit" I mean a commit with no changes in it but we still need a message inside.

"footer": "",
}

commit_mock = mocker.patch("commitizen.git.commit")
Copy link
Member

Choose a reason for hiding this comment

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

@Dranaxel Thanks for correcting me. Yes, you're right. In that case, we probably should assert whether this function is called with --allow-empty. This can be done by something likeassert_called_with.

Copy link
Author

Choose a reason for hiding this comment

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

Sorry i'm not sure about what you mean/how to use assert_called_with. Are you suggesting to assert the flag allow-empty was here when le command was launched ?

Copy link
Member

Choose a reason for hiding this comment

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

assert --allow-empty is actually passed into commitizen.git.commit when the operation above is executed

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.

Need rebase but I am OK with this one. I can definitely see use cases where you just want to force a commit without changes (like forcing rebuild to have updated dependencies).

Note: we might want the bump counter part for the same reason, aka. being able to force a release not containing commits eligible to changelog (aka. #723)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants