-
-
Notifications
You must be signed in to change notification settings - Fork 281
refactor(BaseConfig): update function name, upgrade mypy version #1444
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?
refactor(BaseConfig): update function name, upgrade mypy version #1444
Conversation
def add_path(self, path: str | Path) -> None: | ||
self._path = Path(path) | ||
|
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 just found this method name confusing. It's actually updating the path, right?
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 the main reason we make it this way is that we want to handle both str
and Path
implicitly. We probably could use a setter if that's better
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 I was trying the following
@property
def path(self) -> Path | None:
return self._path
@path.setter
def path(self, path: str | Path) -> None:
self._path = Path(path)
but mypy isn't happy
commitizen/config/yaml_config.py:17: error: Incompatible types in assignment (expression has type "Path | str", variable has type "Path | None") [assignment]
commitizen/config/toml_config.py:17: error: Incompatible types in assignment (expression has type "Path | str", variable has type "Path | None") [assignment]
commitizen/config/json_config.py:16: error: Incompatible types in assignment (expression has type "Path | str", variable has type "Path | None") [assignment]
I'm taking a look at related issues and PRs. Updating mypy to 1.15.0
does not work 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.
Looks like python/mypy#18510 will be part of 1.16.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #1444 +/- ##
==========================================
+ Coverage 97.33% 97.57% +0.23%
==========================================
Files 42 57 +15
Lines 2104 2682 +578
==========================================
+ Hits 2048 2617 +569
- Misses 56 65 +9
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:
|
5aaad88
to
313143d
Compare
cca754e
to
45fe774
Compare
45fe774
to
117e61c
Compare
Description
Checklist
Code Changes
poetry all
locally to ensure this change passes linter check and testsDocumentation Changes
poetry doc
locally to ensure the documentation pages renders correctlyExpected Behavior
Steps to Test This Pull Request
Additional Context