DAS-1892 - Add pycodestyle check to automated test suite. #4
Conversation
| 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 |
There was a problem hiding this comment.
The two changes in this file and varinfo/var_info.py are styling things relating to the binary operator breaking stuff (W503 vs W504).
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
| 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 |
There was a problem hiding this comment.
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?
|
|
||
| * 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 |
There was a problem hiding this comment.
Not a nit or suggestion but what's this binary operator business about haha?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
Description
This PR stems from conversations with @eni-awowale during her work on DAS-1891. There wasn't an automated check for code styling in the regular bunch of unit tests. This PR adds that. (The test mimics others from repositories like HOSS).
Jira Issue ID
DAS-1892 (not strictly, but sort-of related)
Local Test Steps
N/A
PR Acceptance Checklist
Jira ticket acceptance criteria met.CHANGELOG.mdupdated to include high level summary of PR changes.VERSIONupdated if publishing a release.Documentation updated (if needed).