search

bndr / pipreqs

pipreqs - Generate pip requirements.txt file based on imports of any project. Looking for maintainers to move this project forward.
Apache License 2.0
6.38k stars 388 forks source link

Flake8 code quality #274 #276

Open kuldeepkhatke opened 3 years ago

kuldeepkhatke commented 3 years ago

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 commented 3 years ago

Hi @alan-barzilay ,

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

alan-barzilay commented 3 years ago

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?