Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

DAS-1892 - Add pycodestyle check to automated test suite. #4

Merged
merged 2 commits into from
Aug 3, 2023
Merged

DAS-1892 - Add pycodestyle check to automated test suite. #4

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ea71bc8
DAS-1892 - Add pycodestyle check to automated test suite.
owenlittlejohns Aug 2, 2023
a323c20
DAS-1892 - Fix typo in test documentation string.
owenlittlejohns Aug 2, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions dev-requirements.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
coverage ~= 5.5
ipython ~= 8.0.1
jsonschema ~= 4.17.3
pycodestyle ~= 2.11.0
pylint >= 2.5.0
unittest-xml-reporting ~= 3.0.4
31 changes: 31 additions & 0 deletions tests/test_code_format.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
from pathlib import Path
from unittest import TestCase

from pycodestyle import StyleGuide


class TestCodeFormat(TestCase):
""" This test class should ensure all earthdata-varinfo Python code adheres
to standard Python code styling.

Ignored errors and warnings:

* E501: Line length, which defaults to 80 characters. This is a
preferred feature of the code, but not always easily achieved.
* W503: Break before binary operator. Have to ignore one of W503 or
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not a nit or suggestion but what's this binary operator business about haha?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here's the biblical reference.

An example if this is whether you should have:

if (
    my_value == 1.2345
    or my_other_value == 2.3456
):
    ...

Or:

if (
    my_value == 1.2345 or
    my_other_value == 2.3456
):
    ...

Ultimately the answer is that either are okay, so long as the code is consistently one or the other, with PEP008 stating a mild preference for the first option. pycodestyle is automatically configured to warn against both options, which is unhelpful, so the test ignores the checking of the one that is considered slightly preferred.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So either is okay (slight preference for the first) but you should keep it consistent and you turned off the check in pycodestyle for this?

Copy link
Member Author

@owenlittlejohns owenlittlejohns Aug 3, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah - either is okay, but we should choose one and use it throughout the project. Then I turned off the warning for the one we went with. So that second example will still trigger a warning, but the first one won't.

W504 to allow for breaking of some long lines. PEP8 suggests
breaking the line before a binary operator is more "Pythonic".

"""
@classmethod
def setUpClass(cls):
cls.python_files = Path('varinfo').rglob('*.py')

def test_pycodestyle_adherence(self):
""" Ensure all code in the `varinfo` directory adheres to PEP8 defined
standards.

"""
style_guide = StyleGuide(ignore=['E501', 'W503'])
results = style_guide.check_files(self.python_files)
self.assertEqual(results.total_errors, 0, 'Found code style issues.')
4 changes: 2 additions & 2 deletions varinfo/cf_config.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -125,8 +125,8 @@ class object.
mission_matches = re.match(mission, self.mission) is not None

short_name_matches = (
short_name is None or
re.match(short_name, self.short_name) is not None
short_name is None
or re.match(short_name, self.short_name) is not None
Copy link
Member Author

@owenlittlejohns owenlittlejohns Aug 2, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The two changes in this file and varinfo/var_info.py are styling things relating to the binary operator breaking stuff (W503 vs W504).

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh nice! So to test if our code adheres to style requirements we would do something pycodestyle cmr_search.py or can we just run pycodestyle in the root directory and it will check everything?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The style check is now folded into the main test suite, so if you run make test it will run pycodestyle over the whole varinfo directory as part of that command. That's how I found these stragglers. Having that test in place now means that we can't merge the PR until the code is all compliant according to pycodestyle because that will be a test failure.

If you wanted to run it separately, though, you could use pycodestyle in the command line, too. pycodestyle varinfo/*py or pycodestyle varinfo/cmr_search.py as you say.

)

return mission_matches and short_name_matches
Expand Down
10 changes: 7 additions & 3 deletions varinfo/var_info.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -417,9 +417,13 @@ def group_variables_by_horizontal_dimensions(self) -> DimensionsGroupType:
for grid_dimensions, variables in grid_groups.items():
horizontal_dimensions = tuple(
dimension for dimension in grid_dimensions
if (self.get_variable(dimension) is not None and
(self.get_variable(dimension).is_geographic() or
self.get_variable(dimension).is_projection_x_or_y()))
if (
self.get_variable(dimension) is not None
and (
self.get_variable(dimension).is_geographic()
or self.get_variable(dimension).is_projection_x_or_y()
)
)
)

if horizontal_dimensions in horizontal_groups:
Expand Down