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.

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

Fix existing flake8 errors and add flake8 check and verbose flag to CI #237

Merged
merged 7 commits into from
Nov 27, 2017

Conversation

micahkim23
Copy link
Collaborator

@micahkim23 micahkim23 commented Nov 20, 2017

Closes #234

spencerkclark
spencerkclark previously approved these changes Nov 20, 2017
@spencerahill
Copy link
Owner

spencerahill commented Nov 20, 2017

Thanks @micahkim23

So we're getting an error message on the git diff call: https://travis-ci.org/spencerahill/aospy/jobs/304612704#L1101. From that log:

$ git diff upstream/develop **/*py | flake8 --diff --exit-zero || true
fatal: ambiguous argument 'upstream/develop': unknown revision or path not in the working tree.
Use '--' to separate paths from revisions, like this:

But I now see that xarray's tests (from which we stole that line, replacing upstream/master with upstream/develop have the exact same error: https://travis-ci.org/pydata/xarray/jobs/304072446#L4089

The command does work on my local machine, however (with upstream/develop), in that I don't get this error message. But in playing around with it briefly, I couldn't actually trigger a non-zero exit status. I'm new to git diff, so I doubt it's something too difficult. But we should get this sorted out before merging this.

@micahkim23 so you might have to read up on git diff a bit to figure this out.

@spencerkclark
Copy link
Collaborator

Is it simply that there is no remote named upstream on Travis? Agreed that we should try to fix this.

@spencerkclark spencerkclark dismissed their stale review November 20, 2017 16:53

I missed something that I think we should try to fix.

@spencerkclark
Copy link
Collaborator

Naively I might try adding something like:

- git remote add upstream https://github.com/spencerahill/aospy.git

before the following line in our .travis.yml:

- py.test aospy --cov=aospy --cov-report term-missing

@spencerahill
Copy link
Owner

@spencerkclark good idea.

Separately, see #238 re: the test failures

@micahkim23
Copy link
Collaborator Author

micahkim23 commented Nov 26, 2017

I think the appveyor failures will be fixed with #240 and the commit that closes the files in test_calc_basic.py. Should we wait until that pr is merged or should I just make the same change here?

@spencerkclark
Copy link
Collaborator

Thanks @micahkim23! If it's not too much trouble, go ahead and fix the failing Appveyor tests following a3a3753. It will be good to get the flake8 check in before we merge further PR's. I may do some more work in #240, so it might be open a bit longer (and I'm totally cool merging my branch with the latest develop branch once this gets merged).

@spencerkclark spencerkclark changed the title add pep8 to CI and verbose flag to pytest Fix existing flake8 errors and add flake8 check and verbose flag to CI Nov 26, 2017
@spencerahill
Copy link
Owner

@micahkim23 this is great!

go ahead and fix the failing Appveyor tests following a3a3753. It will be good to get the flake8 check in before we merge further PR's

I agree with this. Excited to merge once this is addressed.

@micahkim23
Copy link
Collaborator Author

Now the only appveyor failure is the issue described in #238.

@spencerahill
Copy link
Owner

Now the only appveyor failure is the issue described in #238.

Exactly. So in it goes! Thanks @micahkim23 for the heavy lifting and @spencerkclark for the review

@spencerahill spencerahill merged commit da18455 into spencerahill:develop Nov 27, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants