Add and apply formatters and linters to codebase

[tomvothecoder]

Description

This PR adds popular Python QA packages to the developer environment to improve the software's quality and developer workflow.

Summary of Changes

• Code formatting using black
• Linting using flake8 and isort
• Optional static type checking using mypy
• Enforce and automate standards using above tools with pre-commit
  • To install pre-commit hooks: activate conda environment and run pre-commit install
  • TODO: Update docs with pre-commit installation guide
• Add VSCode .vscode/settings.json file to share IDE configurations -- leverage tools and add editor rulers
• Fix imports in complete_run_v_2_3_0.py due to missing __init__ files
  • Must run with python -m unittest tests/complete_run_v_2_3_0.py, or it will complain about ModuleNotFoundError

Why?

These tools will greatly increase the code quality of e3sm_diags by enforcing and automating project standards, even as the software scales and the number of contributors grow. For more information on these tools, please read my article: Git Pre-commit: Enforce and Automate Repository Standards at the Commit Level.

How?

These tools were applied against the codebase to update the code style and catch PEP8 and static typing issues that were manually fixed/temporarily ignored. Please note, no code logic was changed in the process of formatting and linting so the code should run the exact same as master.

I've also added the pre-commit package, which automates running these tools before every git commit -m .... Docs will be updated in a new PR on how to set up pre-commit.

Example of pre-commit running tools on committed files. I didn't commit anything in this case. If a tool fails, then your commit won't go through, it outputs the error(s), and fixes the errors if it has that capability (e.g. black and isort). Otherwise if everything passes, your commit will go through as normal.

Fixes # (issue)

• Closes Add code formatter and linter to enforce standards #379

Type of change

• New feature (non-breaking change which adds functionality)
• This change requires a documentation update

How to Review This PR

Since applying these tools produces diffs in many files, it will take a bit of time for review. As mentioned before, no code logic was changed so it shouldn't be hard to use the GitHub's 'Files Changed' tool to see the styling diffs. Most of the diffs are produced from line-length limit of 88, changing single quotes to double quotes (a black convention), and isort sorting imports.

---

List of Linting and Static Typing Issues Found and Fixed

isort caught errors for I001 (import order).

isort automatically fixed to follow PEP8 import standard.

flake8 caught PEP8 issues listed below:

• F401 - Module imported but unused
• E741 - Do not use variables named 'I', 'O', or 'l'
• F841 - Local variable name is assigned to but never used
• F632 - use ==/!= to compare constant literals (str, bytes, int, float, tuple)
• E722 - Do not use bare 'except'
• E225 - Missing whitespace around operator
• E265 - Block comment should with '#'
• E231 - missing whitespace after ‘,’, ‘;’, or ‘:’

List of PEP8 error codes here

Most these issues have been fixed, except for F841 errors.
I've added # FIXME comments above these variables and # noqa inline comment to make sure flake8 ignores the error.

Example in meridional_mean_2d_driver.py

# FIXME: F841 - assigned but unused
regions = parameter.regions  # noqa

Next action here is to review # FIXME comments and address these issues in a new PR.

---

mypy caught static type checking issues:

• error: Module has no attribute ...
• error: could not determine type of ...
• No overload variant of "__setitem__" of "list" matches argument types "int", "List[Any]"
• "object" has no attribute "keys"

Some these issues were fixed, but most of them were ignored.
I've added # FIXME comments above these variables and # type: ignore inline comment to make sure mypy ignores the error.

Example in enso_diags_parameter.py

            if not hasattr(self, "test_start_yr"):
                # FIXME: error: Cannot determine type of 'start_yr'
                self.test_start_yr = self.start_yr  # type: ignore

Next action here is to review # FIXME comments and address these issues in a new PR.

---

[tomvothecoder]

@forsyth2 I'll go over this PR in our Monday meeting to provide some more context

[forsyth2]

Looks good as far as I can tell from skimming.

• Squash commits, unless you think they're organized logically
• Your refactoring suggestions can be addressed in another pull request. Best not to further complicate this one.
• Make sure the unit tests pass.

[chengzhuzhang]

Looks good. Your article on pre-commiting is very helpful. As Ryan suggested, we need to make sure all tests pass before merging. At some point, i will consult you about configuring IDE/text editor to use these tools...

[tomvothecoder]

@chengzhuzhang I'm glad to hear the article was helpful!

Once PRs #383 and #385 are merged, I can run the test suite on this PR.

Also I'd be glad to help with your IDE/text editor setup. There is a way to share VSCode workspace settings for the project so that VSCode is automatically configured by opening the file. I can include it in this PR.

[tomvothecoder]

@forsyth2 @chengzhuzhang this PR is ready for final review. The branch is rebased on the latest master and all of the tools have been applied on those commits.

The local test suite and TravisCI build passes.
For the Anvil complete test run, there is 0 regression with the master branch since they both fail with the same 4 mismatching images. Attached are partial logs for each run.
210219_master_complete_run.log
210219_pr_381_complete_run.log

I've included a .vscode/settings.json file to automatically configure VSCode to leverage all of the tools and add editor rulers for comments, max line, and automatic wrapping. You can also edit your workspace or general VSCode settings to set "editor.formatOnSave": true, which formats if there are no errors/warnings. I didn't include that setting based on preference for when you want to format your code.

[chengzhuzhang]

@tomvothecoder Hey Tom, the PR looks good! Regarding to the complete test failure. It looks like the 4 figures in question have pixel difference fraction (although small) larger than the threshold set, so can't be ignored. I think we should visual check the images and maybe to consider to flex the fraction threshold a bit more. Would you point me to the path where the complete test was ran?

[forsyth2]

@chengzhuzhang Sorry, already on that. https://github.com/E3SM-Project/e3sm_diags/pull/373/files#diff-535b9e4349bf24011ce127f7ce530fdbbf007e23486b005b29a5cec32c42b8baR441 changed the units. I'm updating the expected images. I also created #386 to explain how to do this.

[chengzhuzhang]

Makes sense! The units change caused this. The image checker is absolutely useful!

[forsyth2]

@chengzhuzhang #388 makes it so the image diffs will automatically generate. https://web.lcrc.anl.gov/public/e3sm/diagnostic_output/ac.forsyth2/image_check_failures_2022_02_22/MERRA2-TMQ-ANN-global_diff.png is an example of what this looks like and how I could tell the issue was the units

[tomvothecoder]

@forsyth2 The PR has been rebased on the latest master with #388 and the tools have been applied. The local test suite, TravisCI, and the complete test run on Anvil all pass. Please review the changes and let me know if everything checks out.

I've also cleaned up and simplified the PR description.

[forsyth2]

Looks good. If all automated tests and the complete run pass, then this should be good to merge. It would be good to clean up the commits though -- not necessarily squashing into one commit, but at least into logical increments.

[chengzhuzhang]

Hey @forsyth2 I mis-clicked re-requesting review. Please ignore...

[tomvothecoder]

Looks good. If all automated tests and the complete run pass, then this should be good to merge. It would be good to clean up the commits though -- not necessarily squashing into one commit, but at least into logical increments.

Resolved remaining PR issues, all tests and builds pass, and squashed. Merged!