napalm-automation / napalm-base

Apache License 2.0
32 stars 48 forks source link

Fix setup.cfg and requirements.cfg files #61

Open dbarrosop opened 8 years ago

dbarrosop commented 8 years ago

Relates to napalm-automation/napalm#280

Add setup.cfg and requirements-dev.txt file with the following content:

[pylama]
linters = mccabe,pep8,pyflakes
ignore = D203,C901

[pylama:pep8]
max_line_length = 100
pytest
pytest-cov
pytest-json
pytest-pythonpath
pylama
-r requirements.txt
dbarrosop commented 8 years ago

Pinging @napalm-automation/council

What do we want to enforce and what do we don't want to? Let's first decide what we want to achieve and then let's build a baby steps to achieve it. Might make sense to enable things one by one and enforcing rather than just dropping these files.

ktbyers commented 8 years ago

@dbarrosop Here is what I am currently using for my NAPALM pylama.ini that I am 100% compliant with (on ios.py):

[pylama]
linters = mccabe,pep8,pyflakes
ignore = D203,C901

[pylama:pep8]
max_line_length = 120

pep257 (docstring formatting is just too over the top on the OCD; mccabe c901 is too much work to comply with...basically I am not going to do a large amount of code refactoring to comply with mccabe complexity).

You might also want to exclude some directory in the setup.cfg (./build, ./dist, and maybe the ./test directory).

ktbyers commented 8 years ago

My argument against getting rid of PEP257 is that we are currently ignoring the warnings (including warnings that are more important than docstring formatting):

For example: pylama -o ../pylama.ini eos.py | wc -l 170 pylama -o ../pylama.ini iosxr.py | wc -l 287

Also I think it has to be enforced as part of the PR submission process in some way (whatever 'lint' standard we agree on).

Basically, I don't want to be constantly cleaning up other people's 'lint' issues. I get annoyed enough cleaning up my own, but I don't want to have to circle back and cleanup other people's previous PRs.

As @dbarrosop mentioned in a previous post, I would rather have a lower-hurdle that we comply with than just have a bunch of warnings that we ignore.

dbarrosop commented 8 years ago

So, a few things:

  1. First of all I am good if we decide to remove something, that was mostly a proposal trying to spark some discussions.
  2. We should agree on what we want, not what we can support today.
  3. Finally we should do baby steps until we get to where want to go.
  4. The end goal is that travis enforces whatever we decide on every PR, yes. For example, if we decide to go with pylama it's just a matter of adding pylama . right after the tests.
  5. I would rather have max_line_length to 100 if everybody agrees. 120 is a bit too much when travelling and everything you have to code is a laptop

Unless someone has anything else against it we can probably start with your proposal (maybe changing 120 to 100?) and if your code already complies to those specs you can probably add pylama . to the end of the script:section in the travis file.

ktbyers commented 8 years ago

Line length of 100 sounds good. Will update (assuming agreement on this).

I will also probably try to start enforcing this in the travis file shortly.

dbarrosop commented 8 years ago

Great! I am good with your proposal. I have updated the issue accordingly. Let's wait until tomorrow to see if someone else has something to say.

dbarrosop commented 8 years ago

I am assuming this one is good, let's implements what's in the body of the issue : )