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 code quality #274 #276

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

kuldeepkhatke
Copy link

Updated code formatting & long line break changes
Added warning ignore comments at unused imports, duplicate imports, multiple imports
Added line spaces after function definition

@kuldeepkhatke
Copy link
Author

Hi @alan-barzilay ,

Please review this PR & let me know if any changes needed.
Thanks

@alan-barzilay
Copy link
Collaborator

alan-barzilay commented Sep 10, 2021

hi @kuldeepkhatke, thank you for your contribution and welcome to pipreqs!

It seems like your changes broke one test, do you have any clue as to what may have caused this? From what I could gather you only added comments with directives for flake8, this shouldn't affect the unit tests. Did you change anything else that I missed?

Could you also add a comment in the top of the test file with a quick explanation about those directives and where someone could refer to learn more about them? those directives only make sense to someone already familiar with flake8, newcomers will likely get confused by them.
Btw, F401 happens quite a lot, I think it would be better to just pass it as a code to be ignored directly to flake8 in reviewdog

Also, I think your 2 commits could be squashed into one since they all deal with general flake8 fixes.
Could you also change your merge into the "next" branch instead of master? I know they are synchronized right now, but I will make a new release pretty soon and then this will no longer be the case

Sorry about the nitpicking and thanks again for your contribution!

Edit:

Added warning ignore comments at unused imports, duplicate imports, multiple imports

Maybe we should just add all of those directly into review dog instead of only F401. What is your opinion on this?

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

Successfully merging this pull request may close these issues.

None yet

2 participants