Skip to content

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

Closed

Conversation

bearomorphism
Copy link
Contributor

@bearomorphism bearomorphism commented May 29, 2025

Description

Enforce strict type with Pydantic for questions.

Checklist

Code Changes

  • Add test cases to all the changes you introduce
  • Run poetry all locally to ensure this change passes linter check and tests
  • Manually test the changes:
    • Verify the feature/bug fix works as expected in real-world scenarios
    • Test edge cases and error conditions
    • Ensure backward compatibility is maintained
    • Document any manual testing steps performed
  • Update the documentation for the changes

Expected Behavior

Steps to Test This Pull Request

Run cz c -- --allow-empty and finish the conventional commits questions.

Additional Context

class SelectQuestion(QuestionBase):
type: Literal["select"]
choices: list[Choice]
use_search_filter: Optional[bool] = None # TODO: confirm type
Copy link
Contributor Author

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

#1381

Copy link
Contributor

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

Copy link

codecov bot commented May 29, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Please upload report for BASE (refactors@9420b44). Learn more about missing BASE report.

Additional details and impacted files
@@             Coverage Diff              @@
##             refactors    #1467   +/-   ##
============================================
  Coverage             ?   97.85%           
============================================
  Files                ?       58           
  Lines                ?     2652           
  Branches             ?        0           
============================================
  Hits                 ?     2595           
  Misses               ?       57           
  Partials             ?        0           
Flag Coverage Δ
unittests 97.85% <100.00%> (?)

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.



class CzQuestionModel(CzModel):
question: CzQuestion = Field(discriminator="type")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@bearomorphism bearomorphism changed the title refactor(question): strict type with pydantic for questions fix(question): strict type with pydantic for questions May 29, 2025
@bearomorphism
Copy link
Contributor Author

Found related pydantic discussion here: #1073

@Lee-W
Copy link
Member

Lee-W commented May 30, 2025

#1073 (comment)

Yep, back then @woile mentioned there were some incompatibility issues. Not sure how things works now

@bearomorphism
Copy link
Contributor Author

Are there still incompatibility issues if we use pydantic>=1.10,<3?

@woile
Copy link
Member

woile commented May 31, 2025

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

@gbaian10
Copy link
Contributor

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.

@woile
Copy link
Member

woile commented May 31, 2025

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

@bearomorphism
Copy link
Contributor Author

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.

@Lee-W
Copy link
Member

Lee-W commented Jun 1, 2025

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.

Maybe we could mention this in our FAQ as well 🤔

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants