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

WIP: Add type annotations to base spec #1676

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

edgarrmondragon
Copy link
Contributor

@edgarrmondragon edgarrmondragon commented Sep 18, 2024

@edgarrmondragon edgarrmondragon changed the title Add type annotations to base spec WIP: Add type annotations to base spec Sep 18, 2024
@martindurant
Copy link
Member

Thanks for giving this a start. Perhaps you already appreciate that this process will not be easy for the package, and spec might have been the hardest place to start (since all these methods are overridden in various duck ways in the backend implementations).

@scholtalbers scholtalbers mentioned this pull request Nov 7, 2024
@mon
Copy link

mon commented Nov 27, 2024

Preface: Just a random user.

This is a really good start (I love me some types), though I feel like it could do with some minor improvements

  • a few object annotations that could probably be narrowed to a Protocol
  • Inconsistent typing for inferred kwargs (explicitly specified recursive: bool = False vs implied on_error="omit")
  • open could do with an overload to differentiate binary and text streams

...however, I feel like it could just devolve into a bunch of minor nitpicks, whereas this already provides lots of value as-is.

What's the strategy towards a merge here? Grab a bunch of popular downstream consumers and ensure these don't cause conflicts with any overloaded duck typing happening there?

@martindurant
Copy link
Member

@mon : as with my initial comments, I am happy to see typing leak slowly into fsspec, but the duck-type design of the (old!) code means that it will be hard to have it everywhere and certainly very hard to get mypy happy throughout the codebase, including downstream implementations.

It gets complicated fast, because of issues like people demanding support for Path instances where originally only strings were allowed. The end-user API is based on convenience, but library developers using fsspec want strict definitions - requirements that are hard to simultaneously meet.

What's the strategy towards a merge here? Grab a bunch of popular downstream consumers and ensure these don't cause conflicts with any overloaded duck typing happening there?

It shouldn't be possible to break code with types alone, right?
However, I need people more experiences with type-ifying to review the code. It is not for my convenience, but for others, so it will not be high on my priority list.

@mon
Copy link

mon commented Dec 6, 2024

It shouldn't be possible to break code with types alone, right?

At runtime, there will be no effect, unless you type annotate forward/circular references. But these are found as soon as the module is imported, so shouldn't ever be a problem for you, only the person adding the types (and easily fixed by gating the import behind an if TYPE_CHECKING branch).

My worry was there might just be some downstream projects that, for example, extend the valid argument types for some functions, and making them specifically typed (instead of the default Unknown) will cause type errors from tools like Pylance. Personally I don't think that's an issue, and imperfect types is better than none.

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.

3 participants