Conversation
This is the default line length applied by Black. Flake8 is configured in `setup.cfg` to use the same line length.
Codecov Report
@@ Coverage Diff @@
## trunk #422 +/- ##
==========================================
+ Coverage 96.91% 97.55% +0.63%
==========================================
Files 13 13
Lines 551 572 +21
Branches 112 112
==========================================
+ Hits 534 558 +24
+ Misses 9 7 -2
+ Partials 8 7 -1
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
| # Incremental has support for package names | ||
| return version.package | ||
|
|
||
| raise TypeError(f"Unsupported type for __version__: {type(version)}") |
There was a problem hiding this comment.
The previous code would implicitly return None here, but the caller couldn't handler None, so I believe that was a bug.
adiroiban
left a comment
There was a problem hiding this comment.
Looks good to me, but I have little experience with mypy.
I have enabled mypy on CI to make sure we continue to keep annotation in future changes.
Also the new tests looks ok.
src/towncrier/_settings/load.py
Outdated
| def load_config_from_options(directory, config): | ||
| if config is None: | ||
| def load_config_from_options( | ||
| directory: Optional[str], config_path: Optional[str] |
There was a problem hiding this comment.
I guess that this is private API so is ok to rename the argument.
There was a problem hiding this comment.
Mypy doesn't like one variable being used with two different types, so I had to rename either the argument or the local use. As the argument contains the path of the config file rather than the configuration itself, I figured that would be the best one to rename. And as you say, it's inside a private module, so it should be safe.
By the way, does Towncrier even have a public API in the form of Python modules or is the command line interface the only public interface? The docs don't mention any modules and the docstrings are rather sparse.
|
Many thanks @mthuurne for the update. I did a quick review. You can wait for a review or feel free to merge this. Thanks again |
|
I'll wait a day or so for any other devs to give their opinion and merge if I get an additional approval or no response. |
|
I'll try to find more time to look at it, but two things for now:
|
hynek
left a comment
There was a problem hiding this comment.
Looks mostly good, should be ready for merge after a few nits!
I forgot to do this when moving the mypy config into `pyproject.toml`.
hynek
left a comment
There was a problem hiding this comment.
Thanks for this!. Types are a huge boon to a low-churn project like towncrier and will help me with some refactorings I have in mind.
Description
Adds type annotations and checks them using mypy in strict mode.
I omitted the unit tests, as they use Trial's
assertSomething()methods which aren't annotated yet in Twisted, so there would not be much value in annotating the tests at this time.Closes #421
Checklist
src/towncrier/newsfragments/. Describe yourchange and include important information. Your change will be included in the public release notes. (left empty because it's not relevant to end users)