I'd be happy to merge that, and I also start too see why more than 80 characters per line might be a good idea. How about 100 and we see if this goes well?

@janvanrijn any objections on solving all PEP8 issues at once?

@PGijsbers I think it would be most compelling if you could also change the flake8 checking script so that it checks the whole library at once.

Invoking flake8 on all code instead of just diffs seems easy enough. Considering it's a quick operation anyway even for the whole repo, I don't know/think that it's bad practice (especially since you don't need to sort through diffs).
Changing the max line length is also easy if we can agree on it (100 as a start is fine for me).

Codecov Report

Merging #625 into develop will increase coverage by 0.19%.
The diff coverage is 84%.

@@             Coverage Diff             @@
##           develop     #625      +/-   ##
===========================================
+ Coverage    89.77%   89.96%   +0.19%     
===========================================
  Files           32       32              
  Lines         3207     3208       +1     
===========================================
+ Hits          2879     2886       +7     
+ Misses         328      322       -6

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a2a4ade...792adb2. Read the comment docs.

Not sure why the previous tests failed - the changed code only had one minor refactor but might have introduced off-by-one errors (I don't see it)? Even so, the actual numbers are also different in pr and push.

For the particular test it should be that in both cases the most significantly changed line is not even hit. The first call LIMIT is not None evaluates to False, while in the second call it evaluates to True but BATCH_SIZE_ORIG > LIMIT evaluates to False.

This is really cool stuff @PGijsbers , thanks for taking care of this.

I'd be happy to merge that, and I also start too see why more than 80 characters per line might be a good idea. How about 100 and we see if this goes well?

I usually do 120, this seems the default for PyCharm. However keep in mind that for consistency with sklearn 80 is not a very bad idea. (Fine with both)

Flake seems to be failing on warnings, do we want this behavior?
This brings up another issue I noticed (but did not think was very important right now), in that there are two different incompatible warnings: W503 and W504 which forbid a linebreak before and after an operator, respectively (see this).
Do we want to enforce one or the other?
I personally feel that we should not enforce neither, but only set a preference. In my opinion, the preference should be for a line break after the operator, unless it gives a bad visual indent (as is sometimes the case in if-statements for example).

Then I propose the following changes w.r.t. line length:

• I will set the max length to 100.
• For lines longer than 100 characters but shorter than 120 characters, refactoring is encouraged but disabling them with flake with # noqa: E501 is okay too.
• We evaluate at some point if those extra characters are generally warranted or not.

Can either of you two take a look at the failed test? I don't understand how it fails with new code but not old code. As far as I can tell, in neither scenario should BATCH_SIZE_ORIG be modified in the first place, so the lines should not affect the unit test. Moreover it seems that every time the test gets executed, different amount of (increasing) datasets are found. My theory is that someone is simply uploading datasets, and my change got rejected by pure chance? Could this be possible?

I think that this PR should not fail on warnings to keep it manageable.

I think that we should prefer to have operators at the beginning of a line (following math notation).

I'm fine with 100 and 120 character setup.

And @janvanrijn agrees on your assessment of the error in #626 .

I checked the failed tests and it seems to stem from issue #626 and flake8 checking the whole repo (and me only editing the ./openml directory). Looks good to merge to me.