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

#11822 filepath annotations #11823

Merged
merged 24 commits into from
Aug 31, 2023
Merged

#11822 filepath annotations #11823

merged 24 commits into from
Aug 31, 2023

Conversation

glyph
Copy link
Member

@glyph glyph commented Mar 13, 2023

Scope and purpose

Fixes #11822

Contributor Checklist:

This process applies to all pull requests - no matter how small.
Have a look at our developer documentation before submitting your Pull Request.

Below is a non-exhaustive list (as a reminder):

  • The title of the PR should describe the changes and starts with the associated issue number, like “#9782 Remove twisted.news. #1234 Brief description”.
  • A release notes news fragment file was create in src/twisted/newsfragments/ (see: Release notes fragments docs.)
  • The automated tests were updated.
  • Once all checks are green, request a review by leaving a comment that contains exactly the string please review.
    Our bot will trigger the review process, by applying the pending review label
    and requesting a review from the Twisted dev team.

@glyph
Copy link
Member Author

glyph commented Mar 13, 2023

review please

@chevah-robot chevah-robot requested a review from a team March 13, 2023 18:41
@glyph
Copy link
Member Author

glyph commented Mar 13, 2023

haven't run the tests yet myself, but the type-check should be clean

@glyph
Copy link
Member Author

glyph commented Mar 13, 2023

So, those test failures were embarrassing. Let's try that again…

@glyph glyph marked this pull request as ready for review March 13, 2023 22:24
@chevah-robot chevah-robot requested a review from a team March 13, 2023 22:24
@glyph
Copy link
Member Author

glyph commented Mar 13, 2023

Looks like I've got a few minor coverage gaps which should be addressed but otherwise I think we're good now

@glyph
Copy link
Member Author

glyph commented Mar 16, 2023

OK. Coverage gaps all addressed.

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.

I don't have much experience with Python typing.
I did a very quick look and it looks ok
I hope @graingert can also take another look to check all is ok.

Since this is waiting in the queue since March, as long as the tests are green, I think that this can be merged.

Thanks Glyph for the update.

@glyph
Copy link
Member Author

glyph commented Aug 31, 2023

thanks @adiroiban ! I'm happy to take any post-hoc feedback from @graingert (or anyone else for that matter) but as you note, this has been waiting since March, so let's get it out of the queue and move things along, there are plenty of other PRs that need review attention…

@glyph glyph merged commit 1418d41 into trunk Aug 31, 2023
22 checks passed
@glyph glyph deleted the 11822-filepath-annotations branch August 31, 2023 18:57
Comment on lines 276 to 277
def archive(self):
return self
Copy link
Member

Choose a reason for hiding this comment

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

this is the only method not annotated, it should probably be -> Self: # type: ignore[override]

@@ -273,17 +283,87 @@ def _secureEnoughString(path):
return _coerceToFilesystemEncoding(path, secureishString)
Copy link
Member

Choose a reason for hiding this comment

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

this is missing a type annotation - should be def _secureEnoughString(path: AnyStr) -> AnyStr: ...

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.

FilePath and friends should have type annotations
5 participants