Auto flake#1741
Conversation
| expected = {'foo': 'bar'} | ||
| self.dv.encoding['foo'] = 'bar' | ||
| assert expected, self.d == encoding | ||
| assert expected == self.dv.encoding |
There was a problem hiding this comment.
This change was odd. I'm not sure what was expected here.
There was a problem hiding this comment.
Yeah, I think this change is correct. I have no idea what was going on previously -- self.d is not even a valid attribute.
There was a problem hiding this comment.
Previously, it asserted that expected was true-ish; if it wasn’t, it would print the value of self.d == encoding as the message, but that expression is only evaluated if the assert fails.
There was a problem hiding this comment.
I mean, that’s how the code was able to pass without error. Not sure what happened to cause that to be written. 😁
|
I'm not sure how to fix these lines in We're reusing the same key, |
|
@mrocklin oof, you're catching all our questionable code! Try |
| expected = {'foo': 'bar'} | ||
| self.dv.encoding['foo'] = 'bar' | ||
| assert expected, self.d == encoding | ||
| assert expected == self.dv.encoding |
There was a problem hiding this comment.
Yeah, I think this change is correct. I have no idea what was going on previously -- self.d is not even a valid attribute.
| E731, | ||
| # Ambiguous variable names | ||
| E741 | ||
| max-line-length = 120 |
There was a problem hiding this comment.
Let's leave this as the default (79 per PEP8)
| - python -OO -c "import xarray" | ||
| - py.test xarray --cov=xarray --cov-config ci/.coveragerc --cov-report term-missing --verbose $EXTRA_FLAGS | ||
| - git diff upstream/master **/*py | flake8 --diff --exit-zero || true | ||
| - flake8 -j auto xarray |
There was a problem hiding this comment.
question: do we want to have travis fail when flake8 finds something awry?
There was a problem hiding this comment.
IMO yes; it's the only way to keep flake8 passing.
Zac-HD
left a comment
Zac-HD
left a comment
There was a problem hiding this comment.
Generally LGTM, and I think linting everything is a great direction to move in.
However - changing the Travis command and adding exclusions to the config allows us to go backwards as new code is added with warnings that were not previously ignored. See comment below on an alternative approach.
| - python -OO -c "import xarray" | ||
| - py.test xarray --cov=xarray --cov-config ci/.coveragerc --cov-report term-missing --verbose $EXTRA_FLAGS | ||
| - git diff upstream/master **/*py | flake8 --diff --exit-zero || true | ||
| - flake8 -j auto xarray |
There was a problem hiding this comment.
IMO yes; it's the only way to keep flake8 passing.
| script: | ||
| - python -OO -c "import xarray" | ||
| - py.test xarray --cov=xarray --cov-config ci/.coveragerc --cov-report term-missing --verbose $EXTRA_FLAGS | ||
| - git diff upstream/master **/*py | flake8 --diff --exit-zero || true |
There was a problem hiding this comment.
Instead of replacing this, I'd actually enforce that the diff must be flake8-clean:
git diff master **/*py | flake8 --diff
And then add a second check that covers all files but ignores some errors in existing code:
flake8 --ignore E20,E231,E241,E26,E4,E721,E731,E741 xarray
Which means that the config section can (needs to be) be left as-is.
5 participants
flake8 xarraywhats-new.rstfor all changes andapi.rstfor new APII had a free half hour so I decided to run autoflake and autopep8 tools on the codebase.
flake8 xarraypasses. I copied over the exclusions that we use withindask/distributedand extended the line length to 120. You may wish to review these decisions.