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

Extract AbstractSandbox._mk_* methods #4099

Closed
wants to merge 9 commits into from

Conversation

Avasam
Copy link
Contributor

@Avasam Avasam commented Nov 1, 2023

Summary of changes

Prompted by #3979 (comment) . This seems to be the most sensible fix. Outside of a type: ignore.

Pull Request Checklist

@Avasam Avasam mentioned this pull request Nov 1, 2023
2 tasks
@Avasam Avasam changed the title Mark AbstractSandbox._mk_* as static methods Extract AbstractSandbox._mk_* methods Nov 22, 2023
@jaraco
Copy link
Member

jaraco commented Jan 24, 2024

I find it annoying that code that was previously perfectly-valid Python is no longer allowed. I see now that these functions were used locally-only and ignored for their presence in the class. I wonder if the Python language has an opinion on this usage. That is, I'd really like to see an upstream reference (ideally in Python and not just mypy) that clarifies why this usage is discouraged and acknowledges/justifies the weakness(es):

Functions that are used only within the scope of the class declaration and are otherwise unused/ignored after the class is constructed should be declared outside the class. Although this change violates principles of encapsulation, it's preferable to have these declared outside the class scope to avoid implying that such functions are members of the class.

On the other hand, it does appear as if staticmethod is the default condition:

>>> class Foo:
...   def bar(x):
...     print(x)
... 
>>> Foo.bar('foo')
foo

It really feels to me like mypy is broken here, and unless there's upstream guidance as to why the existing form is disallowed or discouraged, let's just suppress the mypy errors with typing: ignore.

@Avasam
Copy link
Contributor Author

Avasam commented Jan 24, 2024

I'm also fine keeping this suppressed in #3979 at the moment.
I will ask around in static-typing chats for opinions/clarifications as you said.

@Avasam
Copy link
Contributor Author

Avasam commented Feb 24, 2024

I'll close this for now, can be re-opened if reconsidered

@Avasam Avasam closed this Feb 24, 2024
@Avasam Avasam deleted the AbstractSandbox-static-methods branch May 9, 2024 20:54
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