-
-
Notifications
You must be signed in to change notification settings - Fork 320
test(init): cover cz without descriptions #1829
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
base: next-release
Are you sure you want to change the base?
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## next-release #1829 +/- ##
=============================================
Coverage 97.98% 97.98%
=============================================
Files 60 60
Lines 2682 2682
=============================================
Hits 2628 2628
Misses 54 54 ☔ View full report in Codecov by Sentry. |
| try: | ||
| # TODO(bearomorphism): can we get the description from the cz class without initiating an instance? | ||
| cz_obj = cz_class(self.config) | ||
| except MissingCzCustomizeConfigError: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, I'm not sure whether this is compatible with plugins. Should we use general exception?
wdyt @Lee-W
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why do you think it might not be compatible
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought other plugins throw other exceptions when they initialize with empty config, but we actually always initialize that with a BaseConfig object.
commitizen/commands/init.py
Outdated
| choices = [] | ||
| for cz_name, cz_class in registry.items(): | ||
| try: | ||
| # TODO(bearomorphism): can we get the description from the cz class without initiating an instance? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
need to make it a class of static method, as we need to take config as an argument, it's not likely.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok
| try: | ||
| # TODO(bearomorphism): can we get the description from the cz class without initiating an instance? | ||
| cz_obj = cz_class(self.config) | ||
| except MissingCzCustomizeConfigError: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why do you think it might not be compatible
commitizen/commands/init.py
Outdated
| # TODO(bearomorphism): can we get the description from the cz class without initiating an instance? | ||
| cz_obj = cz_class(self.config) | ||
| except MissingCzCustomizeConfigError: | ||
| # workaround for cz_customize |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why is it a workaround for cz_customize? I thought it's for non-core czs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do non-core czs throw MissingCzCustomizeConfigError? I am not familiar with it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I understood it correctly, only CzCustomize throws the error
| continue | ||
| first_example = cz_obj.schema().partition("\n")[0] | ||
|
|
||
| # Get the first line of the schema as the description |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we'd better add a description in v5. schema looks like a workaround for me
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it does not have to be a v5 feature. We can make description optional.
84635c9 to
ac05276
Compare
Follow up of #1825
Better test coverage for the change and renamed some variables and methods.