Skip to content

WIP: Fix/issue 251 #279

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

Closed
wants to merge 9 commits into from
Closed

Conversation

Sirius207
Copy link

Description

Follow the discussion of #251, I add the following command.

$ cz undo -h
$ cz undo --bump
$ cz undo --commit

Checklist

  • Add test cases to all the changes you introduce
  • Run ./script/format and ./script/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

cz undo --bump: delete the latest tag and the related commit
cz undo --commit: reset the latest commit

Steps to Test This Pull Request

cz undo --bump

  1. run cz bump
  2. run cz undo --bump
    cz undo --commit
  3. run cz commit
  4. run cz undo --commit

Additional context

If I execute the following statements first, the git.get_tags() would get the tags order by [0.3.0a0, 0.3.0, ...].
image
image
image
So I implement another function git.get_latest_tag()

@woile
Copy link
Member

woile commented Oct 8, 2020

Nice, thanks for this 👍

Could you include a new command documentation in docs/undo.md and update mkdocs.yml with it? Thanks

We are working on fixing the CI issue.

"func": commands.Undo,
"arguments": [
{
"name": ["--bump"],
Copy link
Member

Choose a reason for hiding this comment

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

What do you think of adding a shortcut here? -b and -c for commit?
We'd have cz undo -b

Copy link
Author

Choose a reason for hiding this comment

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

Sounds great 💯

@Lee-W Lee-W mentioned this pull request Oct 8, 2020
4 tasks
@Sirius207 Sirius207 changed the title Fix/issue 251 WIP: Fix/issue 251 Oct 8, 2020
@Sirius207
Copy link
Author

Nice, thanks for this 👍

Could you include a new command documentation in docs/undo.md and update mkdocs.yml with it? Thanks

We are working on fixing the CI issue.

Sure~ 👍

@Lee-W
Copy link
Member

Lee-W commented Oct 8, 2020

@Sirius207 Even though we're working on the CI issue, would you help us ignore commitizen/commands/init.py:50 and fix commitizen/commands/init.py:82? You can ignore line 50 by adding # type: ignore after that line. As for fixing line 82, you'll have to change commitizen/git.py:126 from Optional[List[str]] to List[Optional[str]] and you'll still have to add # type: ignore after line 82.

@Sirius207
Copy link
Author

@Sirius207 Even though we're working on the CI issue, would you help us ignore commitizen/commands/init.py:50 and fix commitizen/commands/init.py:82? You can ignore line 50 by adding # type: ignore after that line. As for fixing line 82, you'll have to change commitizen/git.py:126 from Optional[List[str]] to List[Optional[str]] and you'll still have to add # type: ignore after line 82.

OK 👍

@codecov
Copy link

codecov bot commented Oct 12, 2020

Codecov Report

Merging #279 into master will decrease coverage by 0.20%.
The diff coverage is 91.07%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #279      +/-   ##
==========================================
- Coverage   96.58%   96.37%   -0.21%     
==========================================
  Files          33       34       +1     
  Lines         908      966      +58     
==========================================
+ Hits          877      931      +54     
- Misses         31       35       +4     
Flag Coverage Δ
#unittests 96.37% <91.07%> (-0.21%) ⬇️

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

Impacted Files Coverage Δ
commitizen/cli.py 97.36% <ø> (ø)
commitizen/commands/bump.py 90.58% <ø> (ø)
commitizen/commands/init.py 91.30% <ø> (ø)
commitizen/commands/undo.py 88.23% <88.23%> (ø)
commitizen/git.py 96.00% <90.90%> (+0.49%) ⬆️
commitizen/__version__.py 100.00% <100.00%> (ø)
commitizen/commands/__init__.py 100.00% <100.00%> (ø)
commitizen/commands/check.py 100.00% <100.00%> (ø)
commitizen/cz/base.py 100.00% <0.00%> (ø)
... and 5 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update debe2ce...b3bf009. Read the comment docs.



class Undo:
"""Reset the latest git commit or git tag."""
Copy link
Member

Choose a reason for hiding this comment

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

It seems we use various terms for the same meaning (i.e., reset / revert / undo). I'll suggest unifying them.

Copy link
Author

Choose a reason for hiding this comment

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

I could use "undo". 👌

self.arguments = arguments

def _get_bump_command(self):
created_tag = git.get_latest_tag()
Copy link
Member

Choose a reason for hiding this comment

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

Why is this one named as created_tag instead of latest_tag?

Copy link
Author

@Sirius207 Sirius207 Oct 18, 2020

Choose a reason for hiding this comment

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

The created_tag is original name and get_latest_tag comes later, I would use latest_tag in next commit. 👍

commits = git.get_commits()

if created_tag and commits:
created_commit = commits[0]
Copy link
Member

Choose a reason for hiding this comment

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

same as the question above

return command

def __call__(self):
bump: bool = self.arguments.get("bump")
Copy link
Member

Choose a reason for hiding this comment

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

bump doesn't seem to be clear enough to me. How about something like reverting_bump?

if bump:
command = self._get_bump_command()
elif commit:
command = "git reset HEAD~"
Copy link
Member

Choose a reason for hiding this comment

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

Sometimes we'll encounter the situation that we don't have HEAD. Will it be possible for us to solve that situation?

out.error(c.err)

out.write(c.out)
out.success("Undo successful!")
Copy link
Member

Choose a reason for hiding this comment

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

Maybe Succeeded on undoning! or Undo successfully!?

Copy link
Author

Choose a reason for hiding this comment

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

I prefer the latter 👍

@woile
Copy link
Member

woile commented Feb 9, 2021

Will use documentation for now, closing this

@woile woile closed this Feb 9, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants