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

Inception (languages within languages within...) #696

Merged
merged 12 commits into from
Sep 21, 2020

Conversation

nedtwigg
Copy link
Member

@nedtwigg nedtwigg commented Sep 14, 2020

Implements #412, based on the work we did in #691. This allows users to apply different formatters to specific subsections of code. For example, the following config:

spotless {
  format 'docs', {
    target '*.md'
    withinBlocks 'toLower', '\\n```lower\\n', '\\n```\\n', {
      custom 'lowercase', { str -> str.toLowerCase() }
    }
    withinBlocks 'java only', '\\n```java\\n', '\\n```\\n', JavaExtension, {
      googleJavaFormat()
    }
  }
}

Turns this:

Some stuff
```lower
 A B C
D E F
```
And some java stuff
```java
 public class SomeClass { public void method() {}}
```
And MORE!

Into this:

Some stuff
```lower
 a b c
d e f
```
And some java stuff
```java
public class SomeClass {
  public void method() {}
}

```
And MORE!

…the "store as first step, then restore as last step" functionality. We *could* do it that way, but it's harder to integrate. So we'll just add a second "build" method to the builder, which takes the parameters necessary to make a sub-Formatter.
@nedtwigg
Copy link
Member Author

The problem is, the subrules in a withinBlocks section need to have it. added to them, which is gross. If the it. is left off, the Action closure binds to the parent, which is exactly the opposite of what the user wants. I don't understand why. @bigdaz, do you have any tip for why a groovy closure / Gradle Action would bind to the parent sometimes, but not other times? I have a very short example - if you look at the top of this PR, and you remove all the it., then steps which are meant to be added in a nested way will instead be added to their parent.

The example is taken straight from our unit tests. If you look at the commit right above this comment, you'll see that the tests went from green to red because I removed the it..

@bigdaz
Copy link
Contributor

bigdaz commented Sep 14, 2020

@nedtwigg You're experiencing the default behaviour of Groovy, without any Gradle goodness applied to make it behave like you expect.

Here's my understanding of the behaviour you're seeing:

  • First, Implicit Closure Coercion is converting the Closure provided to an Action type
  • Second, the default Groovy Delegation Strategy kicks in. Method calls are first resolved against the closure owner, then against the delegate. But by default delegate == owner.

The behaviour you've grown to expect from Groovy-with-Gradle is due to the type decoration that Gradle does. Whenever a DSL object is instantiated we transparently add a Closure-accepting method for any method that takes an Action, and we explicitly configure the delegation strategy so that it works in a more "useful" way.

The simple fix for your issue is to let Gradle instantiate the 'sub-extension`:

	private <T extends FormatExtension> void withinBlocksHelper(PipeStepPair.Builder builder, Class<T> clazz, Action<T> configure) {
		// create the sub-extension
		T formatExtension = getProject().getObjects().newInstance(clazz, spotless);
		// configure it
		configure.execute(formatExtension);
		// create a step which applies all of those steps as sub-steps
		FormatterStep step = builder.buildStepWhichAppliesSubSteps(spotless.project.getRootDir().toPath(), formatExtension.steps);
		addStep(step);
	}

To make this work, you'll also need to annotate all of the FormatExtension constructors with @Inject.

@nedtwigg
Copy link
Member Author

nedtwigg commented Sep 14, 2020

Thanks very much for the pointer @bigdaz, that fixed it! Only minor tweak was that it didn't work until I changed it in two places, the place you pointed out above and also here:

@SuppressWarnings("unchecked")
public <T extends FormatExtension> void format(String name, Class<T> clazz, Action<T> configure) {
maybeCreate(name, clazz).lazyActions.add((Action<FormatExtension>) configure);
}
@SuppressWarnings("unchecked")
protected final <T extends FormatExtension> T maybeCreate(String name, Class<T> clazz) {
FormatExtension existing = formats.get(name);
if (existing != null) {
if (!existing.getClass().equals(clazz)) {
throw new GradleException("Tried to add format named '" + name + "'" +
" of type " + clazz + " but one has already been created of type " + existing.getClass());
} else {
return (T) existing;
}
} else {
T formatExtension = instantiateFormatExtension(clazz);
formats.put(name, formatExtension);
createFormatTasks(name, formatExtension);
return formatExtension;
}
}

@jbduncan
Copy link
Member

Oops sorry @nedtwigg, I didn't have the time last weekend to review this - I'm not even sure if I'll have the time next weekend - so if this work needs to be merged in, then please don't wait on me.

@nedtwigg
Copy link
Member Author

No worries! I'll go ahead and merge it before conflicts start to pileup 👍

@nedtwigg nedtwigg merged commit 8b5ad7b into main Sep 21, 2020
@nedtwigg nedtwigg deleted the feat/block-specific-format branch September 21, 2020 12:08
@nedtwigg
Copy link
Member Author

Shipped in plugin-gradle 5.6.1

Copy link
Member

@jbduncan jbduncan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @nedtwigg, I finally got around to reviewing this!

AFAICT everything looks fine other than my comment below; I don't consider this PR controversial, if anything I quite like the idea of doing different formatting inside certain pieces of code, so this PR has my 👍.

withinBlocks 'javascript block', '<script>', '</script>', {
prettier().config(['parser': 'javascript'])
}
withinBlocksRegex 'single-line @(java-expresion)', '@\\((.*?)\\)', JavaExtension, {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not clear on what the regex @\\((.*?)\\) matches. Should we add a comment or replace it with the example in the Javadoc of FormatExtension.withinBlocks?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In some java-html templating systems (I'm thinking of rocker and play framework 2), you can pass java variables to templates, and render any arbitrary expression with @(someExpression), e.g. @(i+1). I'm happy for this regex to get changed to anything else, I personally always find them difficult to read. IMO, the intention is just to show "pass a string and it will be used as a regex".

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahh okay, that makes a lot of sense! Goes to show that I'm not familiar with that class of templating systems. 😛

My opinion is that the example in the doc for FormatExtension.withinBlocks is easier to understand, since it doesn't rely on an understanding of a certain type of templating system, which I think it makes the intention clearer. But I'm happy to defer to you on this.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with you, but not enough to dig up the javadoc, make the change, and push it. If you want to do it, by all means :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Likewise I'm not fussed enough to want to make the change myself, so let's leave it, it's good enough. :)

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

Successfully merging this pull request may close these issues.

3 participants