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

Check style with pep8 & pylint #73

Open
odd22 opened this issue Feb 27, 2018 · 2 comments
Open

Check style with pep8 & pylint #73

odd22 opened this issue Feb 27, 2018 · 2 comments

Comments

@odd22
Copy link
Member

odd22 commented Feb 27, 2018

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
Copy link
Member

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
Copy link
Member Author

odd22 commented Feb 27, 2018

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants