Skip to content

Conversation

@A5rocks
Copy link
Collaborator

@A5rocks A5rocks commented Jan 26, 2025

This PR handles a TODO about making the return types from NodeVisitor explicitly optional. (I'm not personally sure this is the right way to do this given the impact, but I did as the comment said!)

mypy/strconv.py Outdated
if o.metaclass:
a.insert(1, f"Metaclass({o.metaclass.accept(self)})")
inner = o.metaclass.accept(self)
a.insert(1, f"Metaclass({inner})")
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This and similar are a bug due to the context, I think? I would report this as an issue but I ran out of energy while doing this PR.

@github-actions

This comment has been minimized.

@A5rocks
Copy link
Collaborator Author

A5rocks commented Jan 27, 2025

I don't really get why mypyc compiled mypy doesn't work... The stack trace points to mypy.build.build.... which isn't helpful at all. I also haven't tried compiling mypy locally but knowing how compiling things on Windows tends to be, that will be annoying.

@JukkaL
Copy link
Collaborator

JukkaL commented Jan 27, 2025

Having to handle None returns everywhere adds some friction. What about making the base classes use raise NotImplementedError instead of pass?

@JukkaL
Copy link
Collaborator

JukkaL commented Jan 27, 2025

To debug the compiled failures, somebody would have to run the test case in a native debugger. I may look into it later.

@A5rocks A5rocks force-pushed the handle-todo-nodevisitor-returntypes branch from e2acff3 to a06b55d Compare January 28, 2025 01:42
@github-actions
Copy link
Contributor

According to mypy_primer, this change doesn't affect type check results on a corpus of open source code. ✅

Copy link
Collaborator

@JukkaL JukkaL left a comment

Choose a reason for hiding this comment

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

Nice, it's good to finally get rid of the disable_error_code hack.

@JukkaL JukkaL merged commit e046a54 into python:master Jan 28, 2025
18 checks passed
x612skm pushed a commit to x612skm/mypy-dev that referenced this pull request Feb 24, 2025
Raise NotImplementedError by default in NodeVisitor visit methods.
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.

2 participants