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

fix: Restrict static & runtime top-level imports #3482

Merged
merged 24 commits into from
Jul 22, 2024

Conversation

dangotbanned
Copy link
Member

@dangotbanned dangotbanned commented Jul 16, 2024

Fixes: #3478 (comment)
Supersedes: #2927

Description

This PR is primarily fixing pylance autocompletion that has regressed since #3431

However, this also required some runtime changes, which have been spread across commits to more clearly document what is/isn't accessible and where.

  • The main fix comes from 2864c07
  • I'd recommend reviewing the commits sequentially, as a lot of what comes after is organizing/re-writing imports.

Autocomplete

Before

image

After

image

__all__ changes

Every instance of from module_name import * has either:

  1. module_name declares an __all__, which is what gets imported
  2. Every import * can be followed until reaching a defined __all__
2024-07-17.10-45-47.-.Wildcard.follow.mp4

Extra

An added bonus seems to be @deprecated is being recognised in tests/**
Note: deprecated functions still appear, but the visual cue & warning will be displayed

image

This missing is the main source of unpredictable imports.

I've explicitly not included the `Mixin` classes, side effect imports (e.g. `Undefined`, `parse_shorthand`, `@overload`) and `@with_property_setters`
This was the only side-effect import that I imagine was useful
Previously imported in `api` -> here
Currently not included deprecations, but will probably have to revert later
Dropped from top-level:
`DataType`, `TOPLEVEL_ONLY_KEYS`, `mixins`
@dangotbanned dangotbanned marked this pull request as ready for review July 17, 2024 11:50
@binste binste self-requested a review July 17, 2024 17:43
Copy link
Contributor

@binste binste left a comment

Choose a reason for hiding this comment

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

Thanks for taking care of this! I've added one question as a comment. Also, in #3454 (comment) you say that this PR is an opportunity to discuss the boundaries of what we want to consider as breaking. Are you aware of anything in this PR that could be considered a breaking change? Probably the removal of some entries from the top-level Altair namespace? I'm wondering if we want to re-add them and only fix the pylance related autocompletion in this PR. Cleaning up the top-level namespace could be a bigger initiative to tackle for v6. thoughts?

tests/vegalite/v5/test_api.py Show resolved Hide resolved
@dangotbanned
Copy link
Member Author

dangotbanned commented Jul 21, 2024

Thanks for taking care of this! I've added one question as a comment.

Thank you @binste for reviewing!

Also, in #3454 (comment) you say that this PR is an opportunity to discuss the boundaries of what we want to consider as breaking. Are you aware of anything in this PR that could be considered a breaking change? Probably the removal of some entries from the top-level Altair namespace? I'm wondering if we want to re-add them and only fix the pylance related autocompletion in this PR. Cleaning up the top-level namespace could be a bigger initiative to tackle for v6. thoughts?

"Breaking" Changes (alt._ only)

The following are not accessible at the top-level:

  • Mixin classes
    • channels.DatumChannelMixin
    • channels.FieldChannelMixin
    • channels.ValueChannelMixin
  • Module whose members are already accessible at top-level
    • schema.mixins
  • Internal type alias
    • api.DataType
  • Internal decorator
    • schemapi.with_property_setters
  • Internal constant (used in private function)
    • api.TOPLEVEL_ONLY_KEYS

To elaborate on #3454 (comment), this is an effort to define what we consider public API.

I was hoping that by making some opinionated changes, that it would be easier for any maintainer to weigh in with either:

  • a correction (identification of public API)
  • an approval (identification of private API)

So for something like 973dfc7 (#3482) I hoped to open a discussion on

  • Do we expect users to rely on these directly?
  • What is the benefit of exposing such symbols at the top-level?
  • Were these exposed intentionally?

Does that help paint a clearer picture of what I was going for?

@binste
Copy link
Contributor

binste commented Jul 21, 2024

This helps, thanks! May I propose that this PR is changed to only fix the regression in the pylance autocompletion without removing elements from the top-level namespace. I think it's great that you push the discussion forward of what we want to consider the "public" part of the API and what guarantees we offer users. I see a lot of value in defining this.

I'd prefer if we look at defining the public API in one go holistically by going through all elements in a systematic way. By doing it in one go, we can hopefully reduce frictions for end users and provide clear indication of the changes, migration guides, and a guarantee of what is public going forward. It will involve tradeoffs between making Altair better for future users while not breaking too many things for existing codebases and users. Discussing this topic with an overview of all the parts of the package hopefully allows us to get this right.

Thanks again for continue to push Altair in a better direction! Much appreciated :)

@dangotbanned
Copy link
Member Author

Thanks again for continue to push Altair in a better direction! Much appreciated :)

Thank you @binste

This helps, thanks! May I propose that this PR is changed to only fix the regression in the pylance autocompletion without removing elements from the top-level namespace.

That is absolutely fine with me!
I'll work on reverting those changes and simply link to my suggestions in the Motivation section of #3454 for future reference

Copy link
Member Author

Choose a reason for hiding this comment

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

@binste altair.__all__ is now identical to main 👍

@dangotbanned dangotbanned requested a review from binste July 21, 2024 13:28
@binste
Copy link
Contributor

binste commented Jul 22, 2024

That is absolutely fine with me! I'll work on reverting those changes and simply link to my suggestions in the Motivation section of #3454 for future reference

That's perfect, thank you! :)

@binste binste merged commit 2b2f0b8 into vega:main Jul 22, 2024
11 checks passed
@dangotbanned dangotbanned deleted the fix-pylance-imports branch July 31, 2024 12:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants