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

check type of module_all earlier #5789

Closed
wants to merge 3 commits into from
Closed

Conversation

graingert
Copy link
Contributor

@graingert graingert commented Oct 15, 2018

currently mypy.stubgen crashes when processing aiofiles/__init__.py, because __all__ is Tuple[Callable[..., object]] and doesn't continue to process any further modules

See Tinche/aiofiles#53 for the upstream fix

@graingert
Copy link
Contributor Author

Instead it might be better to convert the __all__ to a Option[List[str]] by dropping non-strs

Copy link
Member

@ilevkivskyi ilevkivskyi 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 fixing this! I have two small comments. Could you please also add a test case for this?

module_all is None or
(isinstance(module_all, list) and all(isinstance(v, str) for v in module_all))
):
raise CantImport(module)
Copy link
Member

Choose a reason for hiding this comment

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

This error might be confusing, because we were able to import the module, but it contains a critical error. Maybe add a message (explaining cause) to this exception class?

if not (
module_all is None or
(isinstance(module_all, list) and all(isinstance(v, str) for v in module_all))
):
Copy link
Member

Choose a reason for hiding this comment

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

This statement is a bit hard to read, I think it would be easier if you reformulate it as if module_all is not None and <bad __all__...>.

Copy link
Collaborator

@msullivan msullivan left a comment

Choose a reason for hiding this comment

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

Marking as "Request changes" for tracking purposes.

Do you think you'll have a chance to finish this up?

@msullivan
Copy link
Collaborator

I'm going to close this. Feel free to reopen if you pick it back up.

@msullivan msullivan closed this Oct 18, 2020
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

3 participants