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

Fix to unify type comparison methods #311

Merged
merged 3 commits into from Oct 7, 2022
Merged

Fix to unify type comparison methods #311

merged 3 commits into from Oct 7, 2022

Conversation

daikikatsuragawa
Copy link
Contributor

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

@gaugup
Copy link
Collaborator

gaugup commented Jun 24, 2022

@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?

@daikikatsuragawa
Copy link
Contributor Author

@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.

Signed-off-by: Daiki Katsuragawa <50144563+daikikatsuragawa@users.noreply.github.com>
Signed-off-by: Daiki Katsuragawa <50144563+daikikatsuragawa@users.noreply.github.com>
Signed-off-by: Daiki Katsuragawa <50144563+daikikatsuragawa@users.noreply.github.com>
@daikikatsuragawa
Copy link
Contributor Author

I will fix the failing CI.

This one has been corrected.
bb43f48

@daikikatsuragawa
Copy link
Contributor Author

@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.

flake8 . --count --select=E9,F63,F7,F82 --show-source --statistics

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? 🤔

@amit-sharma
Copy link
Collaborator

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? 🤔

Ah, do you mean that the flake8 still passes when type is used in the code?

@daikikatsuragawa
Copy link
Contributor Author

Yes, that's right.

@amit-sharma amit-sharma merged commit 7a98a0e into interpretml:main Oct 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants