fix: several disabled pylint rules in models/helpers.py #10909
Conversation
kkucharc
force-pushed
the
lint_audit_models_helpers
branch
from
c216cda to
09eccf8
Compare
kkucharc
changed the title
kkucharc
force-pushed
the
lint_audit_models_helpers
branch
from
018d8c3 to
1e76581
Compare
kkucharc
changed the title
kkucharc
changed the title
|
@willbarrett Would you find a time to take a look? |
…not appearing anymore
…n_key_mappings` becomes public because it is accessed by other instance than `self`.
…th check while accessing global context to reset ownerships.
…on column. Removed disabled pylint rule.
kkucharc
force-pushed
the
lint_audit_models_helpers
branch
from
1e76581 to
422c026
Compare
|
@willbarrett Thanks for review. |
| except Exception: # pylint: disable=broad-except | ||
| except (TypeError, JSONDecodeError) as exc: | ||
| logger.error( | ||
| "Unable to load an extra json: %r. Leaving empty.", exc, exc_info=True |
There was a problem hiding this comment.
Can exc or exc_info log data from self.extra_json ?
There was a problem hiding this comment.
Good idea! I will add it to an exception logger.
There was a problem hiding this comment.
maybe I did not explain myself right, sorry. I want to make sure that self.extra_json never gets logged by TypeError or JSONDecodeError by exc or exc_info
There was a problem hiding this comment.
@dpgaspar do you mind to take a look again? I have one doubt: do you think is it possible there could be raised exception while accessing self.extra_json for logger?
There was a problem hiding this comment.
Och, I see, then I just did the opposite thing! :) Reverting this change.
To be honest I think JSONDecodeError will show error message %s: line %d column %d (char %d).
There was a problem hiding this comment.
Just to be sure I check that and any of them won't show what self.extra_json contains.
JSONDecodeError should something like:
json.decoder.JSONDecodeError: Unterminated string starting at: line 1 column 11 (char 10)
with stacktrace.
And TypeError something like this for example:
TypeError: the JSON object must be str, bytes or bytearray, not dict
superset/models/helpers.py
Outdated
| @@ -321,11 +322,11 @@ def template_params_dict(self) -> Dict[Any, Any]: | |||
| return json_to_dict(self.template_params) # type: ignore | |||
|
|
|||
|
|
|||
| def _user_link(user: User) -> Union[Markup, str]: # pylint: disable=no-self-use | |||
| if not user: | |||
| def _user_link(_user: User) -> Union[Markup, str]: | |||
There was a problem hiding this comment.
Can't we just remove the # pylint: disable=no-self-use on this one?
There was a problem hiding this comment.
You are absolutely right! Somehow I had to mistake that for unused-argument. Thanks! I'm changing it right-away.
- removed unecesary function's param rename - added extra JSON content in exception
* Removed conflicting lint and isort check in model helpers seems it's not appearing anymore * Removed disabled linting for accessing private method. `parent_foreign_key_mappings` becomes public because it is accessed by other instance than `self`. * Updated model's helper - removed unecessary exception and replaced with check while accessing global context to reset ownerships. * Updated model's helper - renamed unused attribute to private in user link method. * Updated model's helper - added specific exception for adding extra json column. Removed disabled pylint rule. * Applied changes after review to `models/helpers.py`: - removed unecesary function's param rename - added extra JSON content in exception * Removed self.extra_json content from exception message.
mistercrunch
added
🏷️ bot
SUMMARY
Fixed several disabled pylint rules in models/helpers.py:
[to double backslashBEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
TEST PLAN
ADDITIONAL INFORMATION