Flake8 in travis

[bcipolli]

Review commit-by-commit is recommended...

Major changes include:

• Spacing for math
• Use not in / is not
• Fix up multiline indenting

I also updated tox.ini, so my flake8 Sublime plugin returns meaningful errors on save. Yay!

Note the final commit (e86c0e9) makes an actual code change. Not sure if it's correct.

[effigies]

I think, generally, the idea is not to touch externals, since they're verbatim includes from other libraries. Style-cleaning them becomes a sort of pointless fork that has to be redone every time they're updated.

[bcipolli]

I think, generally, the idea is not to touch externals, since they're verbatim includes from other libraries. Style-cleaning them becomes a sort of pointless fork that has to be redone every time they're updated.

Right, I actually excluded them in tox.ini, but apparently touched them when running the automated commands. I'm working through things carefully myself now! Thanks!

[bcipolli]

Tests are passing, save for a few odd test failures in the bdist_wheel path. Any ideas?

Otherwise, just waiting for feedback on a single style change related to Recorder formatting, then I think we're ready to go!

[matthew-brett]

That error might be a bug in a very recent version of wheel - could try retriggering build.

[matthew-brett]

Or - better - upgrading wheel to latest before running bdist_wheel.

[bcipolli]

Cool, that seems to have done the trick. @effigies had one stylistic objection on the Recoders formatting... otherwise, good to go!

[effigies]

LGTM. Think that pointless line ought to be removed, but definitely not a big deal if that seems out-of-scope for the PR.

[effigies]

It seemed to me that separating flake8 from the regular tests might avoid duplicating work and make it clear from a quick glance whether there's a build or style issue. Take it or leave it, just a suggestion.

New commit: f77b94c
Tests: https://travis-ci.org/effigies/nibabel/builds/107459923

The Travis command is long and ugly. I don't fully understand how to expect conditionals to work, so there might be a cleaner way to do that.

[bcipolli]

👍 thanks!

[bcipolli]

Just cherry-picked the commit; if tests pass, I think we're good!

[effigies]

LGTM. @matthew-brett?

[bcipolli]

If there's any way we could merge this now, it'd be great--I'm working on a number of branches today, and being able to work on this (and have the PEP8 tests / new tox.ini) would be great. Much harder for me to wait and merge later.

Any small issues that come up, I can fix along the way...

[effigies]

Assuming my mandate to review your PRs is still in force, I'm going to go ahead and okay this one.

Thanks a lot.

[matthew-brett]

Chris - thanks for merging this - I couldn't get to it until now, so it's good to keep up the momentum. You certainly do still have my warm approval for your mandate.

In general though - it's good to send out a warning you're going to do the merge, especially if someone has reviewed and not yet obviously signed off - something like - "I'm planning to merge this one soon / tomorrow - any more comments before I do". For something simple like this, half a day's warning is fine, unless there's some obvious unresolved controversy.

[effigies]

@matthew-brett Sounds good.