-
-
Notifications
You must be signed in to change notification settings - Fork 312
fix: try to use ChainMap to chain settings, fix message_length_limit inconsistency
#1812
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: master
Are you sure you want to change the base?
Conversation
message_length_limit inconsistency
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #1812 +/- ##
=======================================
Coverage 97.96% 97.96%
=======================================
Files 60 61 +1
Lines 2648 2657 +9
=======================================
+ Hits 2594 2603 +9
Misses 54 54 ☔ View full report in Codecov by Sentry. |
| from commitizen.defaults import DEFAULT_SETTINGS, Settings | ||
|
|
||
|
|
||
| class ChainSettings: |
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.
Add some docstrings here:
- What is this
- What it does
- How to use
- How it compares to the previous solution
That would really help me understand the benefit.
Something I'd like to know as well:
Right now, if I make a plugin, I can create custom settings, but they are not typed. Would this allow plugin to declare their own settings?
example
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.
Right now, if I make a plugin, I can create custom settings, but they are not typed. Would this allow plugin to declare their own settings?
Thanks, I will put those scenarios into consideration. ChainMap approach should not break the type system.
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.
How it compares to the previous solution
We can discuss this in #1672. I will share the motivation there.
Relate #1672