search

kbase / kb_sdk

Build and test new apps for the KBase platform
http://kbase.github.io/kb_sdk_docs
MIT License
26 stars 32 forks source link

Make python pep8 compliant #344

Closed eapearson closed 4 years ago

eapearson commented 4 years ago

The primary functional changes are in the implementation and server template files, simply making the resulting python code pep8 compliant.

The primary change, numerically, is that the template marker comments did not have a space between the comment start (#) and the first non-whitespace character. Empty line rules were the second most numerous offense. An adjustment was required to to the parser responsible for the impl file so that it can recognize both the old invalid comment markers, as well as properly formatted ones. Without this change the pep8 compliant impl file would trigger an error in kb-sdk. No tests were added, nor were tests run successfully in preparation of this PR. Tests appear to be either broken or difficult to set up correctly, and I did not have time to address this. I've used the resulting changes successfully on a couple of projects.

There is more to do. E.g. src/java/us/kbase/templates/module_python_impl.vm.properties is responsible for generating the initial impl file (and example impl file). The generated impl file is not just pep8 non-compliant but also quite broken and non-functional. Fortunately, running "make" in the module after it is created will rewrite the impl file correctly and install the otherwise missing server file. The module initialization process needs a refactor to ensure that the initial module is good, but that is beyond pep8 compliance and out of scope for the work in this PR.

Also, tests should be fixed or at least fully documented, and test cases adjusted, and pep8/flake8 tests added to ensure the generated code is compliant.

eapearson commented 4 years ago

@ialarmedalien what do you think about merging this? I was about to fix the .travis config so it would build, but when syncing up with develop saw that it was already fixed, and just pushed that on up in to this branch; now it builds. I'm starting back up on the ee2 project, which is what instigated this change, since I was sick of my impl file not linting. Of course, this is the develop branch, but there is very little diff between develop and master, so there is hope.

ialarmedalien commented 4 years ago

This looks good to me. The core change--the regex to search for the parts of the file to replace--should be safe as it searches for specific strings (# ?(BEGIN|END)_HEADER, etc.), so there should be no danger of other random comments accidentally matching.

sychan commented 4 years ago

Are there any other changes required? This just looks like good hygiene fixes.

scanon commented 4 years ago

My only concern is making sure existing SDK modules still work and there isn't some odd mismatch behind the scenes.