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 pkg_resources._declare_state and make it work statically #4258

Merged
merged 2 commits into from May 7, 2024

Conversation

Avasam
Copy link
Contributor

@Avasam Avasam commented Mar 6, 2024

Summary of changes

After playing around with previous PRs with whether _distribution_finders, _namespace_handlers and _namespace_packages definitions should be moved closer to declarations, I noticed that, since we have to define the variables for other reasons anyway, there's no point to _declare_state's global namespace shenanigans. This PR makes it work for static analysis and, after #4246 is merged, will reduce changes in #4242

Pull Request Checklist

  • Changes have tests (existing tests should pass, no intended behaviour change)
  • News fragment added in newsfragments/. (even typing-wise there's no user-facing change here, it's all privates)
    (See documentation for details)

@Avasam Avasam changed the title Type _declare_state and make it work statically Type pkg_resources._declare_state and make it work statically Mar 6, 2024
Copy link
Contributor

@abravalheri abravalheri left a comment

Choose a reason for hiding this comment

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

Thank you very much @Avasam. I think this make sense for me, and it is not a significative change.

The downside is that it does add some "duplication", that the code base so far has been trying to avoid. However, the _declare_state DSL would not be very comfortable for declaring types anyway...

The advantage of this approach is that later we can use to improve the documentation, like proposed in https://github.com/pypa/setuptools/pull/4242/files#diff-e3d3d454fa3a072c9f46f8affa27513892fbc2d245e87f57a5927a7be851de05R100-R106 1, and that has already proven useful to clarify tricky parts like #4249 and improve the understanding on how the code works.

Overall, I think that after we add the correct type hints as in #4242 we are going to gain more than we loose (better clarity/documentation vs. repetition).

(BTW, would it make sense to add the annotations in this PR since it is already introducing the variable declaration?) That would probably showcase the benefits of the approach even more.


@jaraco do you have any thoughts?

Footnotes

  1. Regarding your comment in pkg_resources: Expose the type of imports and re-exports #4242, I think it does make sense to keep the variable type declaration and _declare_state togheter outside of the if TYPE_CHECKING.

@abravalheri
Copy link
Contributor

@Avasam, I am assuming this change does not affect

_declare_state('object', working_set=working_set)
, because
globals().update(locals())
is already ensuring working_set is a global variable, right?

@Avasam
Copy link
Contributor Author

Avasam commented Mar 6, 2024

@Avasam, I am assuming this change does not affect

_declare_state('object', working_set=working_set)

, because

globals().update(locals())

is already ensuring working_set is a global variable, right?

Yep, I validated for working_set too

(BTW, would it make sense to add the annotations in this PR since it is already introducing the variable declaration?) That would probably showcase the benefits of the approach even more.

I know you've already approved the PR, but I added that change since it's small anyway on already modified lines. After #4246, as part of completely typing the register_* methods, I'll introduce some aliases (same that are already in typeshed) to avoid duplicating those Callable type definition

@Avasam
Copy link
Contributor Author

Avasam commented Mar 6, 2024

This change will also reduce errors in #4246 to bring it closer to passing

@abravalheri abravalheri merged commit b3fc00f into pypa:main May 7, 2024
22 checks passed
@Avasam Avasam deleted the non-global-_declare_state branch May 7, 2024 18:42
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.

None yet

2 participants