Whitespace cleanup #1361

Closed
wants to merge 4 commits into from

6 participants

@melezov

This pull request is a general whitespace cleanup, which will benefit users who have automatic removal of whitespaces in their editors - preventing them from creating unwanted changes on save.

Each of lift contributors is a wonderful individual, but their editing programs tend to disagree on the number of spaces a tab represents. This results in a mixture of different indentation standards, sometimes within the same file. The rest of the pull request deals with replacing randomly appearing tabs throughout the framework.

To perform a quick check confirming that there haven't been any code changes you can simply append a ?w=0 at the end of the GitHub diff URL.

Marko Elezovic added some commits Nov 13, 2012
Marko Elezovic Removed all trailing whitespaces from Java & Scala sources. de8be62
Marko Elezovic Replaced leading tabs in Java sources with 8 spaces.
Consolidated EOFs in Java sources with a single trailing newline.
169d410
Marko Elezovic Replaced leading tabs in Scala sources with 2, 4 or 8 spaces, dependi…
…ng on the context.

Consolidated EOFs with a single trailing newline.
56d5697
Marko Elezovic Removed tabs from the README, trimmed LICENSE. 3154080
@fmpwizard
Lift Web Framework member

Thanks for the pull request, this is great and the committers have actually been talking about fixing our formatting a few weeks ago.

Have you used https://github.com/mdr/scalariform ? it would be great if your pull request included integration with the sbt plugin for it, so this will allow us to maintain the formatting style, once we apply it.

So far, the only formatting rule we mostly agreed/talked about was that we don;t want tabs (I see you replaced them for spaces, thanks), and that the indentation should be 2 spaces (as opposed to 4 as some people like)

Thanks!

Diego

@melezov

Heya fmp!

As the Scala style standard defines the indentation as two spaces that is definitely the way to go.

I have used scalariform on many projects in the past, but not so much recently. The problem is that it's just about too much trigger-happy on some hand-crafted code structures and it's a bit of a pain to manually enclose these with a do-not-format comment. That is why I did not deploy it on the entire lift codebase because it would basically mean that there would be no line left intact and I really don't want to be the one responsible for conflicts from hell.

That's why the only thing I did was settle for leading and trailing whitespaces (for now). If anyone's interested there is a really nice Eclipse IDE plugin I use called AnyEdit which automatically manages the whitespaces.

I can add the sbt plugin to the project, but someone else should be the one who pulls the trigger (if the final decision is to actually use the thing).

PS: I suppose it's possible to put a leash on scalariform so that it only performs the same kind of tab replacing / trailing whitespace removal)?

@dcbriccetti
Lift Web Framework member

I haven’t reviewed where Scalariform would make drastic formatting changes in the Lift codebase, but I will say from my own many years of coding experience that I often hand-format something to make it clear, and automatic formatting would ruin this.

@nafg
@andreak
Lift Web Framework member
@melezov

That sounds about resonable, @nafg. I think what would the framework really needs is not some formatting pull request hook but rather

  • a document referring to the style guide
    • a list of lift specific exceptions to the style guide rules such as when writing tests or structures (such as json) which should not adhere to the rules
    • in the document there would be an explanation of why tabs cause problems so that people could understand the issue instead of forcing it upon them
  • optional monthly code-format reviews
    • these are quite easy to do via scalariform or a lift-formatting specific sbt plugin we could write
    • as you mentioned the formatting changes do tend to pollute the development a bit so it's nicer to keep them bundled together
    • during these formatting reviews new best practices could surface up more easily because of deeper understanding of the current state of code and eventual problems the framework code style is experiencing
@Shadowfiend
Lift Web Framework member

+1 on the idea of a style guide.
+1 on the idea of a monthly code review, though we should figure out some way to specifically pick out committers who do it every month or something of the sort, so that there's some sort of schedule.

@fmpwizard
Lift Web Framework member

Alright, so this turned into a nice conversation, what do you think about bringing the conversation to the mailing list, and once we set the styles we want to apply, you create new pull req and we rebase it right away? (because from the time you posted this to now, there have been a few commits, which may or may not have the right spacing, etc).

@melezov

Sounds great, but before we do any rebasing I'd like to invest some time into creating a lift-specific style guide stub document and configuring either a plugin or a command line tool which we could offer inside the pull request as a pre-commit action. Since I'm moderately busy at the moment, I'm targeting the end of week for such an endeavor.

Until then, we can start the discussion on the news group, of course, but I'd rather one of you committers post the topic to engage more people in the discussion. If that's fine with you, we can safely close this issue?

@fmpwizard
Lift Web Framework member

Sorry for the delay on an answer, an email from a committer should not have more importance than an email from a community member, so, please go ahead and start the discussion on the mailing list, and reference this pull request.
I like your idea of first coming up with a style doc, etc and then a new pull request.
Again thanks and I look forward to that thread!

Diego

@fmpwizard fmpwizard closed this Nov 16, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment