interpretml
Fix to unify type comparison methods
This is a modification to unify the type comparison method.
Previously, there were two methods
-
if type(ABC) is str: -
if isinstance(ABC, str):
This fix is related to coding and does not affect services. However, this unification is expected to increase code maintainability during development.
Use isinstance() rather than type() for a typecheck. reference : https://pycodequ.al/docs/pylint-messages/c0123-unidiomatic-typecheck.html
@daikikatsuragawa Thanks for the PR. Could you add a flake8 rule to test for the above as per https://www.flake8rules.com/rules/E721.html? It will be useful for future that we don't add class checks with type. What do you think?
@gaugup
Could you add a flake8 rule to test for the above as per https://www.flake8rules.com/rules/E721.html? It will be useful for future that we don't add class checks with type. What do you think?
Thank you. I will add it. Also, I will fix the failing CI.
I will fix the failing CI.
This one has been corrected. https://github.com/interpretml/DiCE/pull/311/commits/bb43f483b0ffba57568f9c3255b2b6a4b39f6560
@gaugup
Could you add a flake8 rule to test for the above as per https://www.flake8rules.com/rules/E721.html? It will be useful for future that we don't add class checks with type. What do you think?
I was considering adding the following.
https://github.com/interpretml/DiCE/blob/ea6f48472054bd48ec673c1fa1e6bd60ab5a4bef/.github/workflows/python-linting.yml#L32
I tried the following locally, added the error code, and verified that it did not seem to be working properly.
$ flake8 . --count --select=E721,E9,F63,F7,F82 --show-source --statistics
What is it, do you know? 🤔
I tried the following locally, added the error code, and verified that it did not seem to be working properly.
$ flake8 . --count --select=E721,E9,F63,F7,F82 --show-source --statisticsWhat is it, do you know? 🤔
Ah, do you mean that the flake8 still passes when type is used in the code?
