search

openSUSE / obs-service-tar_scm

An OBS source service: fetches code from any SCM and archives it
GNU General Public License v2.0
31 stars 105 forks source link

need better linting #150

Open aspiers opened 7 years ago

aspiers commented 7 years ago

Currently we only have PEP8. flake8 finds other issues, and pylint is even more comprehensive. I've started a linting branch to harness these. The "only" work remaining is to fix all the pylint issues, but there are loads of them.

delirious-lettuce commented 7 years ago

@aspiers ,

I could try and fix some pylint issues but I'm not sure if I should start before #151 is merged?

aspiers commented 7 years ago

@delirious-lettuce On the contrary, we cannot merge #151 until it addresses all the pylint issues, since otherwise Travis will fail on the PR. So your help in finishing #151 would be very welcome! You could simply start with that branch and add a commit for each category of pylint failure, for instance.

delirious-lettuce commented 7 years ago

@aspiers ,

Ah, of course. That makes sense. Looking back at my comment, I think I was confused because there wasn't a linting branch in this repo. I guess I should just fork your fork (where the linting branch is) and send PR's there, right?

aspiers commented 7 years ago

No need to send PRs to my PR :-) Just start with my branch as the base and then do whatever you want to it and push a PR upstream which supersedes #151.

delirious-lettuce commented 7 years ago

@aspiers ,

Sorry for the confusion! That's exactly what I'll do, thanks!

M0ses commented 7 years ago

Hmm, personally I would prefer if we could cleanup master first step by step and then activate pylint/flake8. This way we could distribute the work to more people and could implement new features in the meanwhile. Each module which gets touched while implementing an new bugfix/feature should be sanitized in separate commits inside the PR.

So hopefully we could avoid such a big divergence like we had the last time.

@aspiers What do you think about this?

aspiers commented 7 years ago

Thanks @M0ses, that's a much better idea!

M0ses commented 7 years ago

Some additional thoughts:

The tests directory should not be checked or at least should be checked with a different pylintrc. Problems should be only reported, because we would like / should support different version of pylint (2.6?/2.7/3.x)

For example, these warnings of pyline-3.*

***** Module tests.commontests W: 39, 8: Using deprecated method assertEquals() (deprecated-method) W: 51, 8: Using deprecated method assertRegexpMatches() (deprecated-method) W: 66, 8: Using deprecated method assertRegexpMatches() (deprecated-method) W: 73,12: Using deprecated method assertRegexpMatches() (deprecated-method) W: 83, 8: Using deprecated method assertRegexpMatches() (deprecated-method)

do no affect pylint-2.7 at all, and it should not block us from commiting because the test suite is not "linted".