Apply black to the ml package.
#196
Conversation
|
How should we do this? Should we add our own commits to fix the things we have issues with, or mark them here on Github and @yakutovicha applies the fixes? |
you can just mark the places you don't like and I will disable formatting for them |
|
Many of the changes seem to be due to too long lines. For some of these I would not disable the formatting, but rewrite the line in another way. I guess we can use the code suggestion feature in the comments for those. |
Do you mean the disabled flake8? |
|
Of course, the alternative is to increase the line width to 120 characters. This we can also do. But eventually, we will still hit the "long lines problem". |
Actually, I thought it would be 120. I would put it at least that high. Most screens can hold 150+ chars in half a screen these days (of course it becomes hard to read at some point anyways).
No, I meant just in the Github review comment you can suggest a code commit. |
Done, 120 is now the default. See 2c06483 for the effect.
Sure, that is fine with me too. |
Automatic breaking of long lines is some feature I would perhaps switch of completely. If you don't want to switch it of I would increase the limit very much (180 e.g. ?). I agree screen today are large enough. And I also prefer horizontal scrolling rather than having broken lines which perturb the code flow (it visually disturbs a lot when you are trying to see big-picture i.e. overall structure of the code) |
|
Can we disabled line length checking in order to better see what other changes to the code it does? when I was trying to review it there were so much line-length related changes I was unable to see almost anything else. |
I guess it's a preference thing. I don't really like horizontal scrolling that much, and very long lines can make the cursor jump a lot when navigating by keyboard. I mostly threw the 150 figure because that's about as much text as fits on screen when I code, usually in half screen or a vertically mounted screen. Mostly I wanted to say that the default 80 is way too low for modern equipment. |
|
Can we find a compromise about the line length? I am fine to make it 150 or whatever. But I agree with Nico that horizontal scrolling is not the best. Maybe, if the line is that long, it probably calls for an actual code rewriting. @ProkopHapala would 150 characters be acceptable for you? |
I understand you don't like horizontal scrolling, but usually you don't really need to see all arguments to the function when you read the code. Also how long lines you have in your editor? Don't say you don't fit 180 characters on your screen.
why not 180?
That is exactly why I'm hesitating to use black. I don't want to end up rewriting (re-desiging) the code just to satisfy automatic formater. |
Here are my screenshots. If I have one file open, then it is 263 characters:
When I put two files side by side (which I often do), then it is 119:
If I slightly decrease the font, it becomes 146:
I am not sure why would I say that.
See the numbers above. But if 180 is what you absolutely need, I am fine with 180. |
|
180 is maybe bit on the high side, but it's fine with me. |
|
@ProkopHapala, we are waiting for your decision. We are OK with both: 150 or 180. Please let us know the number so I can continue with the work. |
Aha, OK, sorry. So 180 |
done ✅ |
|
@NikoOinonen I applied the requested changes, please re-review. |
No description provided.