Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

spock data-table formatted incorrectly #822

Closed
AnEmortalKid opened this issue Feb 20, 2019 · 12 comments
Closed

spock data-table formatted incorrectly #822

AnEmortalKid opened this issue Feb 20, 2019 · 12 comments
Labels

Comments

@AnEmortalKid
Copy link

AnEmortalKid commented Feb 20, 2019

Might be related to the way data-tables are dealt with #812

Given this test:

import spock.lang.*

class TestSpec extends Specification {

	@Unroll
	def "Test downstream build"() {

		given:
		println 'nothing'

		when:
		println 'something'

		then:
		println 'nothing #DOWNSTREAM_STEP'

		where:

		DOWNSTREAM                                                                      |DOWNSTREAM_STEP
		[[job: "../some-job/some-branch",parameters: []]] | true
		[
			[job: "../some-job/master", parameters: [[stuff: "blah"]], wait: false ]] | true
		[
			[job: "../some-job/dev"]] | true
		[
			[job: "../some-job/master"],
			[job: "../another-one/master"]] | true
		[
			[job: "     ", parameters: "param", wait: true]] | false
		[
			[job: "", wait: true]] | false
		null                                                                            | false
	}
}

The formatter will mangle the second part of the data-table into a new line:

import spock.lang.*

class TestSpec extends Specification {

	@Unroll
	def "Test downstream build"() {

		given:
		println 'nothing'

		when:
		println 'something'

		then:
		println 'nothing #DOWNSTREAM_STEP'

		where:

		DOWNSTREAM                                                                      |DOWNSTREAM_STEP
		[
			[job: "../some-job/some-branch",parameters: []]]
		| true
		[
			[job: "../some-job/master", parameters: [[stuff: "blah"]], wait: false ]]
		| true
		[
			[job: "../some-job/dev"]]
		| true
		[
			[job: "../some-job/master"],
			[job: "../another-one/master"]]
		| true
		[
			[job: "     ", parameters: "param", wait: true]]
		| false
		[[job: "", wait: true]]| false
		null                                                                            | false
	}
}

This causes things to not compile. I landed here from a bug I raised with spotless: diffplug/spotless#357

I've gotten around this by writing a custom formatter for spotless that basically re-stitches the lines back together (if a line starts with "|" after seeing a where clause)

@eric-milles
Copy link
Member

I will take a quick look at it. The formatter has not received much attention in the past few years. I'm certainly open to helping someone get a workspace set up so they can make regular contributions in that space.

What is the desired outcome of formatting the data table example?

@AnEmortalKid
Copy link
Author

I would settle for either smushing it all into one line:

[[job: "../some-job/some-branch",parameters: []]] | true

Or maybe:

[
  [
    job: "../some-job",
    parameters: []
   ]
] | true

As long as there's no lines starting with | and it compiles I could go either way. My work around just moves the "| true" back on to the previous line.

@eric-milles
Copy link
Member

Here is my before state:
image

And after formatting (no code changes), I am left with this:
image

@AnEmortalKid
Copy link
Author

Odd! I can gather all the versions of the stuff I have for more info.

That formatting looks exactly like what I want! Maybe i just don't have the right formatter...

@eric-milles
Copy link
Member

eric-milles commented Feb 20, 2019

I'm curious why the parser (or the Spock transform) thinks that "[...]\n | true" is not a bitwise-or expression. I'm aware of "'string'\n + ' more'" and "1\n + 2" not parsing as we would expect. But "|" is not a unary operator. But that's a side question...

@AnEmortalKid
Copy link
Author

"[...]\n | true" is not a bitwise-or expression
It might be aware that it's in a where chunk... no idea.

Any versions I can snag to compare formatters? (not sure why yours formats that way but mine doesnt)

@eric-milles
Copy link
Member

To the Groovy parser, there is no awareness of Spock. Spock smarts are applied well after parsing and conversion. "a | b" alone on a line should produce an expression statement with an embedded binary expression that has the operator "|" which is bitwise-or.

Not sure how Spock implements data tables, but in terms of the formatter, that is immaterial.

@eric-milles
Copy link
Member

You mentioned above that spotless is how you are formatting. How does that tool work? What versions of Groovy and Groovy-Eclipse are used?

@eric-milles
Copy link
Member

You can follow and upvote the root cause of your compiler errors here: https://issues.apache.org/jira/browse/GROOVY-9004

@AnEmortalKid
Copy link
Author

AnEmortalKid commented Feb 20, 2019

How does that tool work? What versions of Groovy and Groovy-Eclipse are used?

Spotless calls the groovy eclipse formatter + some custom steps:

The version used is 3.0.0 (at the moment): https://github.com/diffplug/spotless/blob/master/lib-extra/src/main/resources/com/diffplug/spotless/extra/groovy_eclipse_formatter/v4.8.1.lockfile

properties: https://github.com/diffplug/spotless/blob/master/testlib/src/main/resources/groovy/greclipse/format/greclipse.properties

One of the maintainers tried with the latest release and the formatting still looked dodgy.

Eclipse plugins on my end:

image

Version + Build:

Version: 2018-12 (4.10.0)
Build id: 20181214-0600

Groovy/Gradle:

------------------------------------------------------------
Gradle 4.8.1
------------------------------------------------------------

Build time:   2018-06-21 07:53:06 UTC
Revision:     0abdea078047b12df42e7750ccba34d69b516a22

Groovy:       2.4.12
Ant:          Apache Ant(TM) version 1.9.11 compiled on March 23 2018
JVM:          1.8.0_162 (Oracle Corporation 25.162-b12)
OS:           Mac OS X 10.13.6 x86_64

@eric-milles
Copy link
Member

eric-milles commented Feb 20, 2019

These seem like odd choices:

#Whether opening braces position shall be the next line.
#The default value is 'same'.
groovy.formatter.braces.start=next

#Whether closing braces position shall be the next line.
#The default value is 'next'.
groovy.formatter.braces.end=same

What I don't know is how is spotless actually calling the GDT formatter (https://github.com/diffplug/spotless/blob/28a607344ee4d103448329708bdc5dcb84f86297/_ext/eclipse-groovy/src/main/java/com/diffplug/spotless/extra/eclipse/groovy/GrEclipseFormatterStepImpl.java).

And is it using the patched Groovy runtime, which has many parser patches.

@eric-milles
Copy link
Member

For reference, here are the parser patches for the antlr2 groovy parser (search for GRECLIPSE comments):

https://github.com/groovy/groovy-eclipse/blob/master/base/org.codehaus.groovy24/src/org/codehaus/groovy/antlr/groovy.g
https://github.com/groovy/groovy-eclipse/blob/master/base/org.codehaus.groovy24/src/org/codehaus/groovy/antlr/AntlrParserPlugin.java

Core groovy actually has line and column set incorrectly for binary expressions and a number of other expressions. It only needed to be accurate enough for error messaging (aka compiler errors); the formatter requires a much greater degree of accuracy when trying to reposition tokens.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants