mdolab / CMPLXFOIL

GNU General Public License v2.0
29 stars 20 forks source link

Formatting and Linting #6

Closed bernardopacini closed 3 years ago

bernardopacini commented 3 years ago

Purpose

This PR fixes the Python files in this repository to match the MDO Lab formatting (Black) and linting (Flake8) standards. This addresses Issue #5.

NOTE: The complexify.py script cannot be properly handled by Black, presumably because it is very old and not up to Python 3 standards. Since it is currently unusable anyways, I commented out the code within the file and will address the complexify process in a later PR.

Type of change

What types of change is it? Select the appropriate type(s) that describe this PR

Testing

Explain the steps needed to test the new code to verify that it does indeed address the issue and produce the expected behavior.

Checklist

Put an x in the boxes that apply.

bernardopacini commented 3 years ago

It's kinda hard to verify these things without Azure in place. A few things of note:

  • we're not using complexify.py anywhere right? In that case do we want to take it out of the repo? Not a fan of just commenting the file out (renaming the file to complexify or something without the .py extension would be better IMO, but still a bit hacky)
  • there are some poor coding practices, such as from numpy import zeros, but we can fix those in a separate PR (once Azure is in place)

Another option is to hold off on this PR and set up Azure, allowing it to pass even with Flake8 and Black failing. Then we can address Black and Flake8 later with the checks.

For 1) my hope is to refactor that script and make it usable, so I do want to avoid deleting it completely. I can rename it to complexify_dep or something like that so that Black avoids it without commenting out all of the contents. Is this okay with you?

For 2) that is a good point. I can address those in this PR since the Python files are so short -- I don't think it would be confusing.

ewu63 commented 3 years ago

Another option is to hold off on this PR and set up Azure, allowing it to pass even with Flake8 and Black failing. Then we can address Black and Flake8 later with the checks.

Yeah that would be my preference, the earlier we have CI the better we fare.

  1. Yeah I would be fine with that, less diffs than touching the whole file
bernardopacini commented 3 years ago

Another option is to hold off on this PR and set up Azure, allowing it to pass even with Flake8 and Black failing. Then we can address Black and Flake8 later with the checks.

Yeah that would be my preference, the earlier we have CI the better we fare.

  1. Yeah I would be fine with that, less diffs than touching the whole file

Sounds good -- I'll set Azure up and then revisit this. And I'll change the name of complexify.py to avoid the large diff.