Skip to content

duplicate-code triggering on type hints #25

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

Closed
tekktrik opened this issue Nov 15, 2021 · 12 comments
Closed

duplicate-code triggering on type hints #25

tekktrik opened this issue Nov 15, 2021 · 12 comments

Comments

@tekktrik
Copy link
Member

tekktrik commented Nov 15, 2021

Pylint is triggering a failure for duplicate-code on the imports required for type hints. In this case, it triggers on the try-import-except block used to import modules needed only for typing but not operation.

Per conversation, we should look into a way to get avoiding this kind of pylint failure for duplicate code without having to disable it entirely in pre-commit-config.yaml

Current failure when duplicate-code is removed:

************* Module adafruit_funhouse.peripherals
adafruit_funhouse/peripherals.py:1:0: R0801: Similar lines in 2 files
==adafruit_funhouse.__init__:[38:46]
==adafruit_funhouse.network:[37:44]
    from adafruit_dotstar import DotStar
except ImportError:
    pass

__version__ = "0.0.0-auto.0"
__repo__ = "https://github.com/adafruit/Adafruit_CircuitPython_FunHouse.git"

 (duplicate-code)
@tekktrik tekktrik mentioned this issue Nov 15, 2021
@evaherrada
Copy link
Collaborator

@tekktrik Disable it in .pylintrc

@tekktrik
Copy link
Member Author

Will that disable it for whole library? My understanding was that we wanted to keep it working to prevent actual duplicate code, but ignore the try block imports use for type hints, but I'm new to .pylintrc so if there's a way to do that that sounds great.

@tekktrik
Copy link
Member Author

Alternatively, would it help to increase the lines of similarity required up from 4? Not really sure that's a solution so much as potentially decreasing the frequency of this issue. But maybe a small bump up to like 6 would be enough.

@makermelissa
Copy link
Collaborator

Is this still an issue? I just submitted a PR and didn't run into this myself.

@tekktrik
Copy link
Member Author

tekktrik commented Jan 18, 2022

It's currently disabled in the .pre-commit-config.yaml (for all libraries now I believe), so it won't show up now. However, I think the intention was to come up with an alternative solution that would keep this from happening with having to surprises it entirely across libraries. Could we use local pylint suppression perhaps just for the typing try/except blocks? Tagging @kattni who (I think was the one that) asked me to turn this into an issue here.

@kattni
Copy link
Contributor

kattni commented Jan 18, 2022

I wanted issues created for libraries that had actual duplicate code issues because the eventual idea is that they should be refactored to not fail duplicate code. However, that was not meant to be part of the adding type hints project, so I did not expect Tekktrik to complete a refactor. I did not want it disabled in .pylintrc because we'd lose it there. Disabling it in a more obvious place made it simpler to remove later if we follow through with a refactor.

@makermelissa
Copy link
Collaborator

Sounds good. I was just checking to make sure this wasn't supposed to be closed.

@tekktrik
Copy link
Member Author

I think this is resolved because all the libraries including this one got bumped up to 12 lines.

@tekktrik
Copy link
Member Author

Or is this worth still considering refactoring?

@kattni
Copy link
Contributor

kattni commented May 24, 2022

I think it's worth leaving this issue open since it was failing on the code itself before typing was added. If someone wants to look into refactoring this somehow, it might be worth it.

@tekktrik
Copy link
Member Author

Looks like with the increase to 12 lines of similarity, and the fact that the original flag for duplicate code was on type annotations, this actually may be good to close. I re-ran pre-commit without the duplicate-code disable and nothing popped out, and a quick manual look at the code supports that.

@kattni
Copy link
Contributor

kattni commented Jun 15, 2022

Ok, thanks! Closing for now.

@kattni kattni closed this as completed Jun 15, 2022
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

No branches or pull requests

4 participants