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

[8.1] Resolve methods of the REST interface #6873

Closed
wants to merge 3 commits into from

Conversation

atsareg
Copy link
Contributor

@atsareg atsareg commented Mar 1, 2023

This PR avoids exposing methods from parent classes of the REST handler implementation class.

BEGINRELEASENOTES

*Core
FIX: TornadoREST - resolve interface method names for standard HTTP access methods

ENDRELEASENOTES

@atsareg atsareg requested a review from fstagni as a code owner March 1, 2023 09:59
prefix = prefix.lower()
for mName, mObj in inspect.getmembers(cls, lambda x: callable(x) and x.__name__.startswith(prefix)):
methodName = mName[len(prefix) :]
for mName in cls.__dict__:
Copy link
Contributor

Choose a reason for hiding this comment

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

If I understand properly, the only real difference between these two implementations is that you are looping over __dict__ instead of inspect.getmembers, is that correct ?
The logic otherwise seems the same ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, the logic does not change, just extra unnecessary stuff does not get exposed

Copy link
Member

Choose a reason for hiding this comment

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

We already rejected the use of __dict__: #6128

Copy link
Contributor

@chaen chaen Mar 1, 2023

Choose a reason for hiding this comment

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

yes but then the parent class is not exposed, so the extensions are broken

edit: oops, hadn't refreshed, so did not see @chrisburr comment

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