-
Notifications
You must be signed in to change notification settings - Fork 13.9k
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
chore: Simplify views/base #25948
chore: Simplify views/base #25948
Conversation
payload: Optional[dict[str, Any]] = None, | ||
link: Optional[str] = None, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
link
parameter is not used in the codebase at all
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sebastianliebscher it seems like the payload
argument isn't use in the codebase either.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there is a single usage here
superset/superset/views/core.py
Line 166 in d95c200
return json_error_response(payload=payload, status=400) |
) -> Callable[[Callable[..., FlaskResponse]], Callable[..., FlaskResponse]]: | ||
""" | ||
A decorator to set an API endpoint from SupersetView has deprecated. | ||
Issues a log warning | ||
""" | ||
|
||
def _deprecated(f: Callable[..., FlaskResponse]) -> Callable[..., FlaskResponse]: | ||
def wraps(self: "BaseSupersetView", *args: Any, **kwargs: Any) -> FlaskResponse: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this was auto-changed by pre-commit pyupgrade
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @sebastianliebscher for the cleanup. I left a few minor comments, but overall this LGTM.
payload: Optional[dict[str, Any]] = None, | ||
link: Optional[str] = None, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sebastianliebscher it seems like the payload
argument isn't use in the codebase either.
superset/views/base.py
Outdated
""" | ||
Override Response to take into account csv encoding from config.py | ||
""" | ||
"""Override Response to take into account csv encoding from config.py""" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Were these comments manually or automatically changed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Manually. If that change is not desired, I'll revert it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sebastianliebscher could you please revert these docstring changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@john-bodley Done.
The test that is failing is unrelated. There is another PR to address this #25958 |
SUMMARY
Simplifies views/base.py to make code more readable / maintainable by using
pyupgrade
)TESTING INSTRUCTIONS
ADDITIONAL INFORMATION