altair

altair copied to clipboard

@joelostblom Here's a draft PR for type hints just for this one module as a proof-of-concept. Let me know what you think!

Thanks! Note the line at the top of the file: https://github.com/altair-viz/altair/blob/0217b2e73703d3b7b529b73b4dec9c17e7fb09bb/altair/utils/schemapi.py#L1-L2

You should make these changes in https://github.com/altair-viz/altair/blob/master/tools/schemapi/schemapi.py instead, and then run generate_schema_wrapper.py to sync the results to this file.

@jakevdp got it! I'll make that change

@joelostblom @jakevdp Here's the revised PoC

Okay here are some of my attempts; I don't wanna go too much further just in case I'm way off track. Some of the types I have to infer based on my perusal of the code itself, but I may have missed something (e.g. whether or not certain variables are a Dict[str, Any] or SchemaBase object, or both), but you all probably know better

Thanks for making this PR @thewchan and sorry there hasn't been any progress on reviewing it. I think having proper typing support would be great and it would also help us with some issues with tab completion in VS Code that we have been running up against lately (e.g. https://github.com/microsoft/pylance-release/discussions/3709#discussioncomment-5093960).

Do you have time to rebase this against the main branch of the repo, I see there have been some conflicts introduce and I would like to keep this PR alive and mergeable when there is time and expertise to review. Could you also add a py.typed file which seems to be a requirement for supporting types?

@mattijn @ChristopherDavisUCI @binste Do you think this is something we should try to include for 5.0? It seems like a bigger change that should go in a bigger release like 5.0, but we also don't want to delay too long. Maybe it doesn't have to be in the first RC. Also, would anyone of you feel comfortable reviewing this? I am a bit hesitant expertise-wise personally.

I agree, we should definitely try to keep this branch alive as a consistently typed library is great to work with and useful for projects which leverage tools like mypy. Thank you for the effort @thewchan!

However, I don't think this PR is necessary to achieve #2918. I also don't know what the implications for IDEs and type checkers are if we add py.typed. Types are already considered when linting the code base with mypy even without that file and at least pylance uses them as well.

I'd suggest that we look at this in more detail after the release of version 5 and treat it as a bigger typing effort together with #2854 and providing type hints for the complete public interface, not just schemapi. As type hints won't break anything at runtime, maybe it can even be part of a minor release?

@binste @joelostblom

I'm happy to start over given the large amount of changes in the code base. Perhaps it make sense to start adding types incrementally in a way that makes the most sense. Let me know where I should start?

Thank you @thewchan ! @binste do you have a good idea for what the best approach would be here going forward and what would be most helpful for @thewchan to focus on?

I'll soon create a more detailed issue with a suggestion on how we could go about adding type hints to Altair + an overview of the different already opened issues, ideas, etc. I agree with @thewchan that it makes sense to do this incrementally.

Given that you @thewchan already started on schemapi.py it would be great if you could finish the type hints for this file. Happy to review the updated PR. As we get the most out of type hints on the "public api"/often used functionality of Altair, I'd suggest to afterwards tackle altair/vegalite/v5/api.py. Thank you for any contribution you can make to this topic! I'll soon also join in on the effort. Let's use the summary issue (once I've created it) to inform each other which files we are working on so we don't duplicate efforts.

Just fyi, I'm now working on a PR to add mypy as a linter to Altair and fix all current issues so that the current codebase passes.

Thanks again @thewchan for your work! As many things changed in schemapi in the meantime and as this PR didn't see any activity I'm closing it in favour of #3142.