Skip to content
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

Add type annotations #422

Merged
merged 13 commits into from
Sep 6, 2022
Merged

Add type annotations #422

merged 13 commits into from
Sep 6, 2022

Conversation

mthuurne
Copy link
Contributor

@mthuurne mthuurne commented Sep 4, 2022

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

  • Make sure changes are covered by existing or new tests. (mypy checks the annotations)
  • For at least one Python version, make sure local test run is green. (py38, py39 and py310 pass)
  • Create a file in src/towncrier/newsfragments/. Describe your
    change and include important information. Your change will be included in the public release notes. (left empty because it's not relevant to end users)
  • Make sure all GitHub Actions checks are green (they are automatically checking all of the above).

@mthuurne mthuurne requested a review from a team as a code owner September 4, 2022 01:38
@mthuurne mthuurne changed the title 421 type annotations Add type annotations Sep 4, 2022
@codecov
Copy link

codecov bot commented Sep 4, 2022

Codecov Report

Merging #422 (d9eabf0) into trunk (e14a73d) will increase coverage by 0.63%.
The diff coverage is 100.00%.

@@            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     
Impacted Files Coverage Δ
src/towncrier/__init__.py 100.00% <100.00%> (ø)
src/towncrier/_builder.py 100.00% <100.00%> (ø)
src/towncrier/_git.py 100.00% <100.00%> (ø)
src/towncrier/_project.py 80.48% <100.00%> (+9.43%) ⬆️
src/towncrier/_settings/__init__.py 100.00% <100.00%> (ø)
src/towncrier/_settings/fragment_types.py 100.00% <100.00%> (ø)
src/towncrier/_settings/load.py 95.89% <100.00%> (+0.11%) ⬆️
src/towncrier/_shell.py 100.00% <100.00%> (ø)
src/towncrier/_writer.py 100.00% <100.00%> (ø)
src/towncrier/build.py 100.00% <100.00%> (ø)
... and 3 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@@ -76,3 +77,5 @@ def get_project_name(package_dir, package):
if isinstance(version, Version):
# Incremental has support for package names
return version.package

raise TypeError(f"Unsupported type for __version__: {type(version)}")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The previous code would implicitly return None here, but the caller couldn't handler None, so I believe that was a bug.

Copy link
Member

@adiroiban adiroiban left a comment

Choose a reason for hiding this comment

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

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.

def load_config_from_options(directory, config):
if config is None:
def load_config_from_options(
directory: Optional[str], config_path: Optional[str]
Copy link
Member

Choose a reason for hiding this comment

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

I guess that this is private API so is ok to rename the argument.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

@adiroiban adiroiban requested a review from a team September 4, 2022 02:07
@adiroiban
Copy link
Member

Many thanks @mthuurne for the update.

I did a quick review.
I added back the team.

You can wait for a review or feel free to merge this.

Thanks again

@mthuurne
Copy link
Contributor Author

mthuurne commented Sep 4, 2022

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.

@hynek
Copy link
Member

hynek commented Sep 4, 2022

I'll try to find more time to look at it, but two things for now:

  • put the config into pyproject.toml
  • add from __future__ import annotations to all files with type stubs and run Pyupgrade (part of pre-commit)

Copy link
Member

@hynek hynek left a comment

Choose a reason for hiding this comment

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

Looks mostly good, should be ready for merge after a few nits!

MANIFEST.in Outdated Show resolved Hide resolved
src/towncrier/_writer.py Outdated Show resolved Hide resolved
src/towncrier/create.py Show resolved Hide resolved
Copy link
Member

@hynek hynek left a comment

Choose a reason for hiding this comment

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

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.

@hynek hynek enabled auto-merge September 6, 2022 12:54
@hynek hynek merged commit 6c94dc2 into trunk Sep 6, 2022
@hynek hynek deleted the 421-type-annotations branch September 6, 2022 12:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add type annotations
3 participants