Skip to content

Conversation

@asottile
Copy link
Contributor

@asottile
Copy link
Contributor Author

maybe more explicitly this should be Any | None ? though I'm not sure if the BdbQuit part of this is possible?

@AlexWaygood
Copy link
Member

though I'm not sure if the BdbQuit part of this is possible?

yeah, that looks like dead code to me... maybe it's worth adding a comment to the stub? At first glance the runtime implementation definitely looks like it could return None if you ever hit the except branch

@github-actions
Copy link
Contributor

According to mypy_primer, this change has no effect on the checked open source code. 🤖🎉

self, cmd: str | CodeType, globals: dict[str, Any] | None = None, locals: Mapping[str, Any] | None = None
) -> None: ...
def runeval(self, expr: str, globals: dict[str, Any] | None = None, locals: Mapping[str, Any] | None = None) -> None: ...
def runeval(self, expr: str, globals: dict[str, Any] | None = None, locals: Mapping[str, Any] | None = None) -> Any: ...
Copy link
Collaborator

Choose a reason for hiding this comment

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

We also need a comment describing the Any:

Suggested change
def runeval(self, expr: str, globals: dict[str, Any] | None = None, locals: Mapping[str, Any] | None = None) -> Any: ...
# Returns the value of the evaluated expression.
def runeval(self, expr: str, globals: dict[str, Any] | None = None, locals: Mapping[str, Any] | None = None) -> Any: ...

(The globals and locals dict/Mapping are common, obvious expressions and don't need a comment.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

in a twist of irony the signature of eval actually does the opposite -- the comment describes globals and not the return value -- this is just matching the signature of eval which is maybe a better comment? (but I guess then it should be locals: Mapping[str, object]?)

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.

3 participants