search

scalanlp / breeze

Breeze is/was a numerical processing library for Scala.
https://www.scalanlp.org
Apache License 2.0
3.45k stars 693 forks source link

Enforce scalafmt 2.5.2 in travis build #782

Closed bluesheeptoken closed 4 years ago

bluesheeptoken commented 4 years ago

For code consistency I was wondering if we could enforce the use of scalafmt in travis build.

The first commit is an idea of the code after formatting with scalafmt

bluesheeptoken commented 4 years ago

i'll be honest: I really dislike code scala code formatters...

May I ask why ? I have to admit I use it without thinking about the "should I?"

though I definitely like some of these changes. If we could make it a little less opinionated about certain things I'd probably be willing to go for it. Let me look at it more closely in the next week or so

Sure, thanks ! We can probably play with the rules and remove the checks in Travis build !

dlwh commented 4 years ago

First, I'm definitely in the minority on this kind of thing.

I've at least once had scalafmt break code (this was a few years ago at work and they fixed it but it's not great for trust. bugs happen of course).

But frequently I find the way it (and almost any code formatter) groups things makes things less clear.

I'm ok with checks if they're for reasonable things or places where it can't really change how one reads the code. Linting rather than formatting, basically. I like there to be a bit of flexibility.

As a dumb example: I don't particularly like hard length limits on lines. Like, most lines shouldn't go over 100 characters but sometimes (especially with signature boilerplate) it makes things harder to read if you break stuff up too much.

On Fri, May 29, 2020 at 11:19 AM Louis FRULEUX notifications@github.com wrote:

i'll be honest: I really dislike code scala code formatters...

May I ask why ? I have to admit I use it without thinking about the "should I?"

though I definitely like some of these changes. If we could make it a little less opinionated about certain things I'd probably be willing to go for it. Let me look at it more closely in the next week or so

Sure, thanks ! We can probably play with the rules and remove the checks in Travis build !

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/scalanlp/breeze/pull/782#issuecomment-636115649, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAACLING5F3E6QM5KILXJQLRT74E5ANCNFSM4NN2WTUA .

bluesheeptoken commented 4 years ago

I have to admit there are still some issues sometimes. But I have not enough experience with it to have a strong opinion with scalafmt.

Usually I use code formatters on large code base to avoid checking the linting on PR review (for instance missing spaces).

Enforcing format also avoid "quick fixes" on code that does not concern the feature. For example, when i save the file, it adds automatically missing spaces on code I don't work on. Hence, it adds some noise for the review.

I will remove the enforcement on Travis' side. I kinda agree about the line-length rule, it is making the code less clear at several places in this PR.

Thanks for sharing your opinion !