NCAR / ccpp-framework

Common Community Physics Package (CCPP)
http://www.dtcenter.org/community-code/common-community-physics-package-ccpp/
Other
26 stars 64 forks source link

Fix CI test failures in feature/capgen, address pylint warnings #412

Closed climbfuji closed 2 years ago

climbfuji commented 2 years ago

Description

This PR fixes the CI test failures reported in https://github.com/NCAR/ccpp-framework/issues/411.

It also bumps up the pylint rating for test/unit_tests/test_metadata_table.py from below 9 to 10/10 (yay) by removing the unused variable tables in many places.

Caveats

When running

PYTHONPATH=$(pwd)/scripts:$(pwd)/scripts/parse_tools pytest

locally (this is what the GitHub workflow/CI test does), I still get several warnings:

dom.heinzeller@heinzeller-lt:~/work/ccpp-framework/ccpp-framework-feature-capgen-fix-ci-20211102 [gccgfortran-py3-hpc-stack-1.2.0]>  PYTHONPATH=$(pwd)/scripts:$(pwd)/scripts/parse_tools pytest
....                                                                                                                                                                                                                                               [100%]
==================================================================================================================== warnings summary ====================================================================================================================
scripts/state_machine.py:165
  /Users/dom.heinzeller/work/ccpp-framework/ccpp-framework-feature-capgen-fix-ci-20211102/scripts/state_machine.py:165: DeprecationWarning: Flags not at the start of the expression '(?:(?i)timestep_init' (truncated)
    regex = re.compile(value[2] + r"$")

scripts/state_machine.py:166
scripts/state_machine.py:166
scripts/state_machine.py:166
scripts/state_machine.py:166
scripts/state_machine.py:166
  /Users/dom.heinzeller/work/ccpp-framework/ccpp-framework-feature-capgen-fix-ci-20211102/scripts/state_machine.py:166: DeprecationWarning: Flags not at the start of the expression '([A-Za-z][A-Za-z0-9_' (truncated)
    function = re.compile(FORTRAN_ID + r"_(" + value[2] + r")$")

scripts/state_machine.py:165
  /Users/dom.heinzeller/work/ccpp-framework/ccpp-framework-feature-capgen-fix-ci-20211102/scripts/state_machine.py:165: DeprecationWarning: Flags not at the start of the expression '(?:(?i)timestep_fina' (truncated)
    regex = re.compile(value[2] + r"$")

scripts/state_machine.py:165
  /Users/dom.heinzeller/work/ccpp-framework/ccpp-framework-feature-capgen-fix-ci-20211102/scripts/state_machine.py:165: DeprecationWarning: Flags not at the start of the expression '(?:(?i)init(?:ial(?:' (truncated)
    regex = re.compile(value[2] + r"$")

scripts/state_machine.py:165
  /Users/dom.heinzeller/work/ccpp-framework/ccpp-framework-feature-capgen-fix-ci-20211102/scripts/state_machine.py:165: DeprecationWarning: Flags not at the start of the expression '(?:(?i)final(?:ize)?' (truncated)
    regex = re.compile(value[2] + r"$")

scripts/state_machine.py:165
  /Users/dom.heinzeller/work/ccpp-framework/ccpp-framework-feature-capgen-fix-ci-20211102/scripts/state_machine.py:165: DeprecationWarning: Flags not at the start of the expression '(?:(?i)run)$'
    regex = re.compile(value[2] + r"$")

-- Docs: https://docs.pytest.org/en/latest/warnings.html
4 passed, 10 warnings in 0.08s
climbfuji commented 2 years ago

The issue before was not the whitespace change but why you are choosing to add unrelated changes to a bugfix PR. To quote a developer I know:

I think we should try to be good stewards of GitHub code change practices and create smaller PRs that contain logically related changes but nothing else / not much else otherwise going forward.

Except that there is a difference between removing a newline when changes that are bug fixes do shorten the lines so that they fit within reasonable limits or othewise require adjusting the indentation for the second line (and all this on 19 lines in a single file), and thousands of lines of code changes that are unrelated in one PR ...

At any rate, I committed your suggestion and changed all other occurrences to match it. Note that your suggested change also removed the newline (https://github.com/NCAR/ccpp-framework/pull/412/commits/45415b5c1d1b2bb37c8b7312ce2bdfc8229d58f4).

gold2718 commented 2 years ago

Except that there is a difference between removing a newline when changes that are bug fixes do shorten the lines so that they fit within reasonable limits ...

My issue is that unless we have a definition of "reasonable limits", we will keep having this problem.