Begin checking that python code meets PEP8 standard and pass pylint or flake8

[edwardhartnett]

The PEP8 style guide was put together in 2001 by a group including Guido van Rossum, python's author. (See also How to Write Beautiful Python Code With PEP 8).

Following PEP8 guidelines is extremely helpful for keeping maintenance costs down. By following consistent code conventions, new programmers will find it easier to deal with the code, and bad code practices will be eliminated.

PEP8 is commonly used on python projects. (I have never worked on a python project which did not use it.)

PEP8 standard can be enforced by either pylint or flake8. Pylint is somewhat stricter on your code, flake8 enforces the PEP8 standards and nothing else. (Current code fails either. See edwardhartnett#1 for example.)

The way forward would be to:

• Decided to follow this quality standard.
• Bring existing code up to standard to get a pass.
• Run pylint/flake8 in CI, to ensure all new python code follows PEP8.

After that, it's no additional work. Programmers will have to follow PEP8 to get their PR to pass CI.

I highly recommend that you follow PEP8. It is very usual for python projects and will result in more readable code with lower maintenance costs.

[edwardhartnett]

[BenjaminBlake-NOAA]

@edwardhartnett I'll work on bringing the existing code up to PEP8 standards. It'll definitely be good to have this in place going forward.

[edwardhartnett]

OK, I have done a little refresh and it seems like flake8 is the thing to start with.

I will put up a PR with flake8 testing.

Some fixes (maybe all) can be made without testing. I would suggest you go through the list and do the easy ones first. Any that require more code changes, maybe write a test first.

[guoqing-noaa]

@edwardhartnett It will be good if we can also run this tool offline before creating a PR (for CI testing)

[edwardhartnett]

Indeed you can. Take a look here: https://github.com/edwardhartnett/rrfs-workflow/actions/runs/10724678590/job/29740852415?pr=1

I have added a workflow which runs flake8 before running pytest. Take a look at the workflow file to see what it is doing.

It installs flake8 with pip, and then checks the code in two passes:

# stop the build if there are Python syntax errors or undefined names
  flake8 . --count --select=E9,F6,F7,F82 --show-source --statistics
  # exit-zero treats all errors as warnings. The GitHub editor is 127 chars wide
  flake8 . --count --exit-zero --max-complexity=10 --max-line-length=127 --statistics

I didn't come up with that, it was in the GitHub default CI for python applications. You can look up all the arguments to flake8 and adjust if you like.

So the CI is failing on the first check, due to lots of undefined names and some syntax errors around print statements...

[edwardhartnett]

OK, today I realized that this is python2 code, and both I and PEP8 were expecting python3 code.

Are we staying with python2 on rrfs-workflow? Or is python3 a goal not yet reached?

[BenjaminBlake-NOAA]

@edwardhartnett I honestly thought that python 3 was being used, and python/3.8.6 is loaded at runtime. Are all the scripts/code you see in the workflow still using python 2? I think that is undesirable since python 2 is no longer supported and we should use python 3. @JacobCarley-NOAA @MatthewPyle-NOAA do you agree?

[MatthewPyle-NOAA]

We definitely should be using python 3

[MatthewPyle-NOAA]

@BenjaminBlake-NOAA Not sure if taking a look at the current SRW App would be worthwhile, but might be a good reference.

[JacobCarley-NOAA]

Yes, we should be using python 3. There are tools out there that can help with converting if need be, like 2to3. 2to3 should be available in our version of python.

[BenjaminBlake-NOAA]

@JacobCarley-NOAA I was just going to mention the 2to3 utility. It is available on WCOSS2 and all the Python scripts/code could easily be converted to python3. I can do this soon.

[edwardhartnett]

The reason I think at least some of the code is python2 is because of the print statements.

We have print statements like:

print "something"

which is valid in python2, but a syntax error in python3.

I agree that python3 is the correct choice.

[BenjaminBlake-NOAA]

@edwardhartnett Agreed, I think a lot of the differences will be related to the print statements. Thanks for bringing this up! Once I'm able to convert everything to python3 let's revisit adding these unit tests.

[edwardhartnett]

I have a fix for all the print problems and I can put up a PR for it.

I just wanted to confirm that python3 was the goal...

[edwardhartnett]

OK, I've put up a PR which fixes the syntax errors, and turns on syntax error checking in the CI, to prevent all further syntax errors from being merged.

In the CI, you will see the line:

    flake8 . --count --select=E9 --show-source --statistics

Add to the "select" parameter to check for more than just syntax errors. Aim for:

--select=E9,F63,F7,F82
Add entries one by one, and clean up the warnings as you go.

[edwardhartnett]

OK, now that we are not getting any E999 from flake8 (syntax error), the next on the list is F63, that is, all the warnings on the list that start with F63. The only one we have is invalid-print-syntax, and only in one file, rocoto_viewer.py.

I have a PR with this fix on my fork. Shall I submit it to the main repo?

[BenjaminBlake-NOAA]

@edwardhartnett That would be great. Thanks!

[edwardhartnett]

OK, I've put up that PR. See #477

[BenjaminBlake-NOAA]

Great, thank you. I just saw you opened #478 as well for adding a Python test.

[edwardhartnett]

OK, the goal here was to introduce pylint and flake8, and set them up in CI, and that has been done.

There is still work needed to fully meet PEP8 standards, but I will handle that as separate issues as we progress through the modernization of the python code...