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

Resolves #10283; check for module level ignore_missing_imports on missing stubs #10546

Closed
wants to merge 1 commit into from

Conversation

TheCleric
Copy link
Contributor

@TheCleric TheCleric commented May 28, 2021

Description

Fixes #10283

For missing type stubs for legacy bundled packages, we will now look for module level overrides for ignore_missing_imports. If present, we will not issue an error like: Cannot find implementation or library stub for module named "redis.sentinel"

Test Plan

3 new test scenarios were added for this PR in check-modules.test for a missing type stub for a legacy bundled package:

Test Name ignore_missing_imports global ignore_missing_imports module override Result
testIgnoreMissingImportsGlobalWithMissingStub True False Error
testIgnoreMissingImportsWildcardWithMissingStub False True No Error
testIgnoreMissingImportsGlobalAndWildcardWithMissingStub True True No Error

The first two tests were passing before the code change and are there to help ensure no regression. The last test was the use case from #10283 to make sure the new code accomplished its mission.

# To see if this ignore_missing_imports was due to the global or due to the
# module, let's copy the options, set global ignore_missing_imports to False
# and then look again.
options_copy = copy.deepcopy(manager.options)
Copy link
Collaborator

Choose a reason for hiding this comment

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

This seems potentially dangerous, since Options can have significant state (_per_module_cache) and it's hard to reason about the performance and correctness implications of the cache. I came up with a simple alternative approach that doesn't use deepcopy, and since there is not much time before the release, I'll create a PR now (but I'll give you credit for the first implementation).

@JukkaL
Copy link
Collaborator

JukkaL commented Jun 4, 2021

Thanks for the PR! I'm closing this in favor of #10582 (see my comment above) primarily since options cloning has previously been a real performance bottleneck for very large projects, and I want to be careful not to regress performance.

@JukkaL JukkaL closed this Jun 4, 2021
@TheCleric
Copy link
Contributor Author

Thanks for the PR! I'm closing this in favor of #10582 (see my comment above) primarily since options cloning has previously been a real performance bottleneck for very large projects, and I want to be careful not to regress performance.

Completely understand. I wasn't super happy with my approach anyway. 😂

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.

--ignore-missing-imports not working for modularized typeshed types
2 participants