-
-
Notifications
You must be signed in to change notification settings - Fork 298
fix(question): strict type with pydantic for questions #1467
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
Conversation
cf6a7ec to
4e18ff9
Compare
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 confirm if this is correct. Ping @Yusin0903 @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.
Hi, I checked the questionary.select function signature and found that use_jk_keys, use_search_filter should be bool, not Optional[bool].
Reference: https://github.com/tmbo/questionary/blob/master/questionary/prompts/select.py
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@ ## refactors #1467 +/- ## ============================================ Coverage ? 97.85% ============================================ Files ? 58 Lines ? 2652 Branches ? 0 ============================================ Hits ? 2595 Misses ? 57 Partials ? 0
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
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.
4e18ff9 to
c9b2ba7
Compare
Found related pydantic discussion here: #1073
Yep, back then @woile mentioned there were some incompatibility issues. Not sure how things works now
738fd78 to
148babc
Compare
Are there still incompatibility issues if we use pydantic>=1.10,<3?
What's the benefit of adding pydantic as a dependency? Couldn't we use dataclasses/typeddict and protocols? The user interaction with commitizen barely includes user input, which is already validated by questionary. Therefore, type check at runtime is barely needed 🤔
We already have issues like this: #1474
What's the benefit of adding pydantic as a dependency? Couldn't we use dataclasses/typeddict and protocols? The user interaction with commitizen barely includes user input, which is already validated by
questionary. Therefore, type check at runtime is barely needed 🤔We already have issues like this: #1474
Does this issue point to the wrong link? The issue description is requesting an upgrade of the Ruff version.
Also, I agree that using dataclass instead of Pydantic whenever possible might be a better approach, as it helps avoid unnecessary dependencies.
I guess I misinterpreted the ticket. I thought it was creating an issue on a downstream project.
My point is that a ticket is a burden on (volunteer) maintainers. Because we pin dependencies, if users add commitizen as a dev-dependency, we limit what they can install. pydantic is a widely used dependency, we would have to rush into supporting v3 (depending on how we pin it). This PR doesn't make use of much iterfaces from pydantic, but by having it available, we are telling other contributors: yes, you can use the full pydantic featureset. It's a long term issue FMPOV
This would be a clearer issue:
#1249
I guess I misinterpreted the ticket. I thought it was creating an issue on a downstream project.
My point is that a ticket is a burden on (volunteer) maintainers. Because we pin dependencies, if users add commitizen as a dev-dependency, we limit what they can install. pydantic is a widely used dependency, we would have to rush into supporting v3 (depending on how we pin it). This PR doesn't make use of much iterfaces from pydantic, but by having it available, we are telling other contributors: yes, you can use the full pydantic featureset. It's a long term issue FMPOV
This would be a clearer issue: #1249
You're right. I intention of this PR was that I just wanted a more strict type for Questions. The runtime check is not needed.
Let me close this PR and use TypedDIct to address the issue.
I guess I misinterpreted the ticket. I thought it was creating an issue on a downstream project.
My point is that a ticket is a burden on (volunteer) maintainers. Because we pin dependencies, if users add commitizen as a dev-dependency, we limit what they can install. pydantic is a widely used dependency, we would have to rush into supporting v3 (depending on how we pin it). This PR doesn't make use of much iterfaces from pydantic, but by having it available, we are telling other contributors: yes, you can use the full pydantic featureset. It's a long term issue FMPOV
This would be a clearer issue: #1249You're right. I intention of this PR was that I just wanted a more strict type for
Questions. The runtime check is not needed.Let me close this PR and use
TypedDIctto address the issue.
Maybe we could mention this in our FAQ as well 🤔
Uh oh!
There was an error while loading. Please reload this page.
Description
Enforce strict type with Pydantic for questions.
Checklist
Code Changes
poetry alllocally to ensure this change passes linter check and testsExpected Behavior
Steps to Test This Pull Request
Run
cz c -- --allow-emptyand finish the conventional commits questions.Additional Context