search

liormizr / s3path

s3path is a pathlib extension for AWS S3 Service
Apache License 2.0
206 stars 39 forks source link

:sparkles: Add types for all the public API #132

Closed gabrieldemarmiesse closed 1 year ago

gabrieldemarmiesse commented 1 year ago

Related to #128

gabrieldemarmiesse commented 1 year ago

I like the from __future__ import annotations but unlike other __future__ it's not 100% sure that it will be the default in the future. See https://docs.python.org/3/library/__future__.html#id2 . If it gets rejected, then we will have to change the whole codebase so I'd prefer to avoid using it.

nlangellier commented 1 year ago

I like the from __future__ import annotations but unlike other __future__ it's not 100% sure that it will be the default in the future. See https://docs.python.org/3/library/__future__.html#id2 . If it gets rejected, then we will have to change the whole codebase so I'd prefer to avoid using it.

Makes sense. I'm not strongly opinionated on this.

gabrieldemarmiesse commented 1 year ago

Done @liormizr could I have another code review? I encourage your to use squash and merge as the merging option because otherwise master will be very messy, there are lots of commits in my pull request.

gabrieldemarmiesse commented 1 year ago

hi @liormizr I had some time this weekend and typed the whole public api. I also fixed some return types with Self that will now return the correct type if there is some subclassing (otherwise the type is not narrowed down, see https://peps.python.org/pep-0673/ ).

I also don't really understand why the unit tests are failing because I just changed the types of the functions, it should have no impact on unit testing.

liormizr commented 1 year ago

@gabrieldemarmiesse I added to master a fix for the issue that broke the tests Can you please merge from master so we will rerun the tests?

Thanks

gabrieldemarmiesse commented 1 year ago

@liormizr done!

gabrieldemarmiesse commented 1 year ago

@liormizr can you re-review?

gabrieldemarmiesse commented 1 year ago

@liormizr is there something I can do to push this forward? I think I've adressed all the issues you mentionned.

liormizr commented 1 year ago

Hi @gabrieldemarmiesse Sorry for keeping you waiting I had two crazy weeks Our local PyCon conference in one week, and after a personal vacation. I'll do a last quick view on the PR and update you on Sunday