Skip to content

Conversation

Lee-W
Copy link
Member

@Lee-W Lee-W commented Nov 8, 2019

#47

  • Changes
    • Make commands required
    • Move --version into a command

In the original design, there are 3 arguments --version, --debug, --name NAME.
The later two commands usually come with commands (e.g., ls, commit).
Therefore, I make --version a command.

There are two concern for me in this PR.

  1. Change api without bumping the version
  2. I'm not sure whether catching the TypeError handles the case that command is not given.

@codecov-io
Copy link

codecov-io commented Nov 8, 2019

Codecov Report

❗ No coverage uploaded for pull request base (master@976573e). Click here to learn what that means.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master      #48   +/-   ##
=========================================
  Coverage          ?   93.15%           
=========================================
  Files             ?       25           
  Lines             ?      555           
  Branches          ?        0           
=========================================
  Hits              ?      517           
  Misses            ?       38           
  Partials          ?        0
Impacted Files Coverage Δ
commitizen/cli.py 84% <100%> (ø)
commitizen/commands/__init__.py 100% <100%> (ø)
commitizen/commands/version.py 100% <100%> (ø)

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 976573e...8ae5d8b. Read the comment docs.

@woile
Copy link
Member

woile commented Nov 8, 2019

What problem do you see with --version?

By default if no command is provided the help is shown, check the first comment inside the main in cli.py

@Lee-W
Copy link
Member Author

Lee-W commented Nov 8, 2019

If we pass -n <anything> or --debug without any command, it will throws and exception.
exception

But it won't happen, if we use the argument --version.

Here are my goals of this change.

  1. Make providing command required.
  2. Make the behaviors of providing arguments similar.

@woile
Copy link
Member

woile commented Nov 9, 2019

In order to avoid a breaking change we can add a warning for the --version and in the next major release, deprecate it. What do you think?

import warnings

warnings.warn("--version will be no longer available in next major release", category=DeprecationWarning)

@Lee-W
Copy link
Member Author

Lee-W commented Nov 9, 2019

In this case, maybe we can create another PR for the warning and add version to command. We can merge this PR when 2.0 is to released. Does it make sense to you?

@woile
Copy link
Member

woile commented Nov 9, 2019 via email

@Lee-W
Copy link
Member Author

Lee-W commented Nov 9, 2019

Yes, we'll have both.
But we cannot have subcommand required and --version exist at the same time.
We'll have to remove the subcommand required constraint so that we can have both.

What's in my mind is how to remind us that we need to add back the constraint when we're to release 2.0.
That's why I want this PR to stay as it is and create another one for coexisting of --version and version but no contraint.

@woile
Copy link
Member

woile commented Nov 11, 2019

Currently, if no command is provided, the user will see the "help" information, do we want to show an error instead? I don't see the use of this constraint, why do you think it would be useful to us? Thanks

@Lee-W
Copy link
Member Author

Lee-W commented Nov 11, 2019

Because if we provide argument name or debug without command (i.e. cz -n cz_jira, cz --debug), it will throw an exception like the following which I think should give the user at least the "help" information.
image

@woile
Copy link
Member

woile commented Nov 12, 2019

Okay got it, makes sense. I think we can comment the line and add a TODO: Add constraint back in 2.0. It's the only thing I can think of right now.

@Lee-W
Copy link
Member Author

Lee-W commented Nov 13, 2019

I've add the TODO and add --vesrion back.

@woile woile merged commit c100cf7 into commitizen-tools:master Nov 13, 2019
@Lee-W Lee-W deleted the make-command-required branch December 18, 2019 08:42
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