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

Type information for RunGroupBy.apply() #258

Open
mikapfl opened this issue Sep 12, 2023 · 5 comments
Open

Type information for RunGroupBy.apply() #258

mikapfl opened this issue Sep 12, 2023 · 5 comments

Comments

@mikapfl
Copy link
Member

mikapfl commented Sep 12, 2023

Is your feature request related to a problem? Please describe.

I get the error:

error: Call to untyped function "apply" in typed context  [no-untyped-call]

if I use run.groupby(…).apply(…). That's a bit sad.

Describe the solution you'd like

It would be great to add type information to RunGroupBy.apply(), however that's not really possible because there would be a dependency cycle: run.groupby() returns a RunGroupBy object (and therefore has to import scmdata.groupby), and RunGroupBy.apply() return an ScmRun object (and therefore has to import scmdata.run).

I'm not sure how to properly solve this.

Describe alternatives you've considered

We could:

  • dump everything into one file, no dependency cycles ever
  • Protocolize ScmRun. Sounds painful and like a lot of work, but would decouple implementation and types clearly.
  • Protocolize RunGroupBy. Probably less work, but I'm not sure if run.py can live without importing scmdata.groupby, so probably doesn't break the cycle?
  • Other ideas?
@znichollscr
Copy link

Is it not possible to use from __future__ import annotations combined with if TYPE_CHECKING to get around this?

@mikapfl
Copy link
Member Author

mikapfl commented Sep 12, 2023

Hm, maybe it is. I think I tried using quotes around types (the equivalent of from __future__ import annotations), but I didn't try if TYPE_CHECKING, so maybe the combination can do the trick.

@lewisjared
Copy link
Collaborator

I think the TYPE_CHECKING guard should work.

I don't think `RunGroupBy is well-typed currently either.

RunGroupBy could use a generic with a TypeVar bound to a BaseScmRun (I think I've used the poorly named T variable). Then the type on run.groupby would be RunGroupBy[Self] and the apply functions would know which type they should return.

@mikapfl
Copy link
Member Author

mikapfl commented Sep 13, 2023

The generic TypeVar could work, but binding it to a BaseScmRun would introduce dependency circles again, no? We could just not bind it at all, which I think would be fine, no one is instantiating RunGroupBys out of whole cloth anyway.

@lewisjared
Copy link
Collaborator

Added typehints in #262

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

No branches or pull requests

3 participants