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

pkg_resources type the declared global variables #4267

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

Avasam
Copy link
Contributor

@Avasam Avasam commented Mar 8, 2024

Summary of changes

Moved declarations of @_call_aside defined globals closer to where they're defined. And after ResourceManager & WorkingSet to avoid forward reference issues
Extracted from #4242 Works towards #2345 , resolves a few reportOptionalCall issues that #4192 would raise

Pull Request Checklist

  • Changes have tests (existing runtime tests should pass, unblocks more type-checking PRs)
  • News fragment added in newsfragments/.
    (See documentation for details)

Comment on lines +3314 to +3331
if TYPE_CHECKING:
# All of these are set by the @_call_aside methods above
__resource_manager = ResourceManager() # Won't exist at runtime
resource_exists = __resource_manager.resource_exists
resource_isdir = __resource_manager.resource_isdir
resource_filename = __resource_manager.resource_filename
resource_stream = __resource_manager.resource_stream
resource_string = __resource_manager.resource_string
resource_listdir = __resource_manager.resource_listdir
set_extraction_path = __resource_manager.set_extraction_path
cleanup_resources = __resource_manager.cleanup_resources

working_set = WorkingSet()
require = working_set.require
iter_entry_points = working_set.iter_entry_points
add_activation_listener = working_set.subscribe
run_script = working_set.run_script
run_main = run_script
Copy link
Contributor

Choose a reason for hiding this comment

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

My question here is:

Instead of "re-declaring" the global variables, wouldn't it make more sense to ditch the @_call_aside and just run the majority of the body of _initialize* functions directly on the global scope?

@jaraco, do you know if _call_aside()/_initialize_master_working_set()/_initialize() are used in pkg_resources just for the sake of organising the code, or is this very small delay on the execution a trick that solves a specific problem?

(I do like the way the code is organised right now, but if changes are needed for type-checking, I would prefer not having double book keeping. Of course we can decide that the type-checking here is not worth the trouble).

Copy link
Contributor Author

@Avasam Avasam May 7, 2024

Choose a reason for hiding this comment

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

I would prefer not having double book keeping

FWIW, you already have to for other checkers (just as a note, most of these variables were moved from the top of the file, so this PR doesn't introduce double book-keeping).

That being said, I agree it'd be nicer if we didn't have to, but I wouldn't want to revamp pkg_resources too much outside of making it a fully typed package to take it out of typeshed. Unless it's a simple change.

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