konstructs / server

A voxel based game server.
http://www.konstructs.org
MIT License
49 stars 12 forks source link

Add scalafmt and enforce in travis #143

Closed petterarvidsson closed 7 years ago

petterarvidsson commented 7 years ago

Let's add this when all other branches are merged and then also run the formatter. In my tests it has been doing a very nice job.

petterarvidsson commented 7 years ago

@nsg What do you think?

petterarvidsson commented 7 years ago

As you can see travis is already failing the code due to bad formatting :smile:

nsg commented 7 years ago

@petterarvidsson is it possible to get a little better output? The stack trace is not telling me that much.

petterarvidsson commented 7 years ago

@nsg since it is a formatter there is no need. Running sbt scalafmt fixes it directly in the code. If you are interested in the error, just check the diff. Basically this check just makes sure that you did run scalafmt before pushing.

nsg commented 7 years ago

@petterarvidsson I see, do not like tools that "helps" you ... because it tend to change things all over the place ... prefer to get a report and if I so choose fix it autocratically or manually.

petterarvidsson commented 7 years ago

@nsg Then we have to drop it. It's an automatic formatter, not a syntax checker. Scalastyle does that. http://www.scalastyle.org/

I personally like automatic formatting much better than syntax checkers since it both enforces and helps with keeping a common style. But there will always only be one correct formatting allowed, which some people may find limiting.

nsg commented 7 years ago

I see ... I have used automatic formatters once, in a go project and I hated it ... I'm completely fine with tools that checks and enforce coding style/formatting but I have never liked tools that with out asking rewrites my code.

Have you used scalastyle? scalafmt? Any opinions?

petterarvidsson commented 7 years ago

@nsg I have only used scalastyle and scalariform. Scalariform is an older formatter which even lacks a test mode so we had to make a diff after we run sbt and check if the code changed to be able to have travis fail. It worked great though! Scalafmt seems much better. scalastyle is a good checker / linter, but it doesn't provide any help with the formatting, which sucks a bit. Basically, if you use a formatter you have to go all in, since otherwise it will start reformatting stuff that you didn't change.

petterarvidsson commented 7 years ago

That's why if we use the automatic formatter, we have to have travis check it, or one could check in code that someone else who runs it will update, which is totally annoying.

nsg commented 7 years ago

@petterarvidsson yeah, I'm a little worried that it will "randomly reformat" a lot of irrelevant LOC that a new release has a new better algorithm to fix. I'm willing to test this, as long we keep the auto-formatting-of-irrelevant-lines as a separate commit, marked as so.

nsg commented 7 years ago

That's why if we use the automatic formatter, we have to have travis check it, or one could check in code that someone else who runs it will update, which is totally annoying.

Agree

petterarvidsson commented 7 years ago

@nsg When I used it in the way described, random updates never happened.

nsg commented 7 years ago

Is the formatting output stable between releases? No, the formatting rules will evolve even between PATCH releases. I recommend you inspect the diff for every scalafmt update.

They will if we upgrade, so, we need to remember that. But guess that's a minor problem.

I'm fine to try it ... not saying that I like it but I guess it will add move value than horror so :)

petterarvidsson commented 7 years ago

Yes, when you update rules or the plugin you make a single commit to sync the source with the new format.

nsg commented 7 years ago

Exactly

nsg commented 7 years ago

Before we merge this, make sure that we have changed the default 80 chars to 120.

petterarvidsson commented 7 years ago

@nsg I think this is ready for merge now!

nsg commented 7 years ago

@petterarvidsson looks good :+1: