Investigate formatter-maven-plugin for CI builds

[ansell]

Investigate adding formatter-maven-plugin to CI builds to validate formatting and fail CI builds (after compile/tests complete) if it has not been applied:

http://code.revelc.net/formatter-maven-plugin/formatter-maven-plugin/plugin-info.html

This will help with issues where the user has accidentally forgot to apply the formatting before submitting their pull request.

[ansell]

In particular, what are peoples thoughts on exporting the formatter configuration as an artifact to Maven Central and putting it on a different build number (similar to some parent pom systems where they only use 1/2/3/4/etc.) so it can be consistently used and tracked separate to the rest of RDF4J?

That is the recommended method for using the formatter-maven-plugin with multi module projects:

http://code.revelc.net/formatter-maven-plugin/formatter-maven-plugin/examples.html#Multimodule_Configuration

[barthanssens]

Related to #790

[abrokenjester]

Fwiw I have previoulsy briefly looked at using the maven checkstyle plugin for this purpose, but got mixed results. It's easy enough to set up but I had a hard time getting it to play nice with the Eclipse built-in code formatter. This plugin looks interesting though, and more immediately suited to our purpose. Link to github repo: https://github.com/revelc/formatter-maven-plugin

[abrokenjester]

I had a bit of a play with this formatter and I really like it. Easy to integrate in our build process, and also plays nice with Eclipse's internal formatter.

[hmottestad]

@jeenbroekstra I tested the formatter on fluent APIs like .stream() and it's a bit aggressive:

return m
    .stream()
    .map(st -> st.getObject())
    .filter(o -> o instanceof Literal)
    .map(l -> (Literal) l)
    .findAny();

Gets converted to a single line, because it's shorter than 120 characters!

I strongly prefer the multiline version of fluent apis, unless there is a particular need for using a one-liner (eg. in an IF condition).

[abrokenjester]

The problem with that is that that's not a distinction the formatter can make. I can configure it to forcible always turn qualified invocations like this into multiline statements (even if < 120 chars) but I can't make an exception for "when it is part of an IF-condition". The upshot is that if I change this, we also get this:

if (book.stringValue()
				.equals("http://example.org/book/book6")) {
			assertEquals("Harry Potter and the Half-Blood Prince", b.getValue("title")
					.stringValue());
		} else if (book.stringValue()
				.equals("http://example.org/book/book7")) {
			assertEquals("Harry Potter and the Deathly Hallows", b.getValue("title")
					.stringValue());

instead of:

		if (book.stringValue().equals("http://example.org/book/book6")) {
			assertEquals("Harry Potter and the Half-Blood Prince", b.getValue("title").stringValue());
		} else if (book.stringValue().equals("http://example.org/book/book7")) {
			assertEquals("Harry Potter and the Deathly Hallows", b.getValue("title").stringValue());

[abrokenjester]

I believe your particular example is an edge case: it's a fluent call that is just under 120 chars. If you feel that having it as a oneliner is less readable, you of course always have the option to use slightly longer variable names. Not only will that improve general readability, but it will also push it past the line length limit, so it will automatically get reformatted.

[hmottestad]

Is there not a way to make it one-way. >120 always become multi line but <120 are ignored?

[hmottestad]

So that if I make it multiline for readability, then it stays multiline.

[abrokenjester]

I'll do one tweak though: the current settings would reformat as follows:

	return model.stream()
			.map(st -> st.getObject())
			.filter(o -> o instanceof Literal)
			.map(l -> (Literal) l)
			.findAny();

I'll change it so it becomes:

	return model
                            .stream()
                            .map(st -> st.getObject())
                            .filter(o -> o instanceof Literal)
                            .map(l -> (Literal) l)
                            .findAny();

(ignore the slightly too large indentation, that's a copy/paste issue)

[hmottestad]

Shame it can’t handle that edge case. Using longer variable names is quite the hack to bypass the auto formatted.

[abrokenjester]

Is there not a way to make it one-way. >120 always become multi line but <120 are ignored?

Afraid not. Or, well, there is, but enabling that would mean that the formatter never joins up two lines anymore. Which rather defeats the purpose.

This kind of thing is always a compromise.

[abrokenjester]

Shame it can’t handle that edge case. Using longer variable names is quite the hack to bypass the auto formatted.

I don't really see it as a hack. If you are aiming for increased readability of your code, longer variable names are exactly a means to that end. And for what it's worth, I actually think that

return m.stream().map(st -> st.getObject()).filter(o -> o instanceof Literal).map(l -> (Literal) l).findAny();

still reads just fine.

[abrokenjester]

Ah, I'm afraid I can't make the change I promised above either - it results in some rather weird side effects, for example:

                           Arrays.asList(new ListBindingSet(bindingNames,
                                           SimpleValueFactory.getInstance().createLiteral("1", XMLSchema.STRING))));

becomes:

                           Arrays
                                           .asList(new ListBindingSet(bindingNames,
                                                           SimpleValueFactory.getInstance().createLiteral("1", XMLSchema.STRING))));

[hmottestad]

That’s fine.