FRRouting / topotests

Moved to frrouting/frr
22 stars 26 forks source link

Check style with pep8 & pylint #73

Open odd22 opened 6 years ago

odd22 commented 6 years ago

In the same order of checkpatch for frr, I think we also need to verify topotest Pull Request, at least with pep8 and pylint. But, running both on my local copy of topotest give very low score.

I discuss this with one of my colleague (PTL for functest at OPNFV). He told me that the topotest directory was not conform to python rules. He recommends me to look at [https://github.com/opnfv/functest] and try to organize topotest in the same way. Once done, we could use tox to automate python style checking same way we are checking C style with checkpath.

Of course, this imply a certain amount of work (not too many said my colleague) to re-factor topotest code according to python rules.

What do you think about this idea ?

rzalamena commented 6 years ago

I think this is a good initiative, because there were no concerns about running linters on the code yet (besides lib/topogen.py). Here are some highlights:

@odd22 said: He recommends me to look at [https://github.com/opnfv/functest] and try to organize topotest in the same way. Once done, we could use tox to automate python style checking same way we are checking C style with checkpath.

I think this is an intrusive change at this time (we have a peak activity at the moment) and it would side track the current incoming tests. Beyond that there is little gain at this point, because no tests are following the pylint rules.

@odd22 said: In the same order of checkpatch for frr, I think we also need to verify topotest Pull Request, at least with pep8 and pylint. But, running both on my local copy of topotest give very low score.

For this exact same reason I think we should slowly start changing the current test to start conforming with the pylint rules and then include it in the quality procedure.


We also have very few people working on topotests, but we welcome people that want to provide code.

Here is my suggestion of action plan:

  1. Update old topologies test to use topogen and make them look similar to the new ones;
  2. Adopt a topology test to start fixing the linter warning/errors until the score becomes good (> 6);
  3. When we stabilize topotest (no more PRs activity), re organize the structure;
  4. Write the code to run the linters on PR and give contributors the feedback;
  5. Update documentation;

If someone is willing to contribute, feel free to ask me for suggestions on where to start.

odd22 commented 6 years ago

@rzalamena Agree, but I would suggest to reconsider part of step 3 as initial step because the renaming of directory and building the python package are mandatory for next steps (tox, linters ...)

We could help and bootstrap this change.