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

flake8 F401 error in __init__.py #1551

Closed
zsef123 opened this issue Aug 28, 2017 · 5 comments
Closed

flake8 F401 error in __init__.py #1551

zsef123 opened this issue Aug 28, 2017 · 5 comments

Comments

@zsef123
Copy link
Contributor

zsef123 commented Aug 28, 2017

Hi, I'm try to refactoring code style.

In PR #1550

piskvorky 22 hours ago • edited Owner
-1: we don't want to litter the code with some tool-specific constructs.

Instead, open an issue with the tool that requires this, it's a bug on their side if they cannot handle this line.

If you find such comments in other parts of gensim, please remove them too.

Same case in sklearn_api/init.py

ex ) from .ldamodel import LdaTransformer # noqa: F401

For solve F401 error, add noqa comments or add __all__ list in __init__.py file.( pyflake issue , stackoverflow question )

Which is better in both cases?

@piskvorky
Copy link
Owner

piskvorky commented Aug 28, 2017

Thanks for digging into this! That pyflake issue really explains it well.

It still looks like a bug in pyflake to me. We could add __all__ (it's better than the weird # noqa comment), but it's still extra code for no reason, so I'm not very thrilled about it. Unnecessary chance to forget to add something there in the future and add more maintenance. @menshikh-iv thoughts?

@menshikh-iv
Copy link
Contributor

menshikh-iv commented Aug 28, 2017

So, for my opinion, noqa is OK.
We can also add __all__ directive, but we should "duplicate" import each time in init (I means we should do 2 things: import somestuff and add somestuff to __all__)

@zsef123
Copy link
Contributor Author

zsef123 commented Sep 5, 2017

@piskvorky @menshikh-iv
PR #1550
+# flake8: noqa

piskvorky 4 days ago Owner
This seems to combine the worst of both worlds. Still extra comments, AND no style checking.
I think I prefer even the noqa:F401 version.

How about ignore F401 error for the __init__.py file in flake8_diff.sh?

In else block, if __init__.py then add F401 on ignore options, otherwise execute original command

Also, If think that noqa is still the better option, I want to close this issue.

@zsef123 zsef123 reopened this Sep 5, 2017
@piskvorky
Copy link
Owner

Alright, let's use the noqa.

@menshikh-iv
Copy link
Contributor

@zsef123 agree with @piskvorky, let's use noqa explicitly.

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

3 participants