Skip to content

Support running with Gradle JAVA_HOME under java 7 #8

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

Closed
wants to merge 2 commits into from
Closed

Support running with Gradle JAVA_HOME under java 7 #8

wants to merge 2 commits into from

Conversation

gaborbernat
Copy link

So I did it, but man it required a lot of modifications 😨 it turns out a lot of Java 8 only features have been used, however now it works just fine under java 7. I think building the release packages should be done with Java 7 in order for that to work.

PS. For easier reviewing I suggest enabling https://reviewable.io/ :) for the repo

Review on Reviewable

@nedtwigg
Copy link
Member

Good work!

I've got two problems with this code:

  1. The commit that converts Java 8 stuff to Java 7 also does a lot of field reorganizing, etc. This means that it will be very difficult to either maintain the Java 8 and and Java 7 branches separately, or to convert the project back to Java 8 by reverting just the "8 to 7" commit in 9 months when the base requirement raises to Java 8 anyway.

  2. The code doesn't port to Java 7, it makes it canonical Java 7.

The old code took any Function<String, String> as a formatter. The great thing about Java 8 is that when you need a function that takes X and returns Y, you just say Function<X, Y>. In Java 7, you've gotta make some class, and anyone reading the code has to go and read that class to figure out "Oh, this just takes an X and returns a Y".

The biggest problem with this code is that it takes things that were once stateless functions, and adds an "init" function to them:

Function<String, String> became FormattingOperation, and now has an init() method.
Supplier<Function<String, String>> became FormattingOperationSupplier, which also has an init() method.

I really don't like init() methods, because it adds something the programmer has to keep track of. When do I call this? Do I have to call it? Does the framework call it? What should go in the constructor and what should go in init()? With a simple Function, there's no ambiguity.

Some of this is inherent to a Java 7 backport, but not all of it. Another way to do this port would be to create a Function<X, Y> interface rather than to create two new classes with init() methods. Something like retrolambda could also reduce the code change.

I can think of two ways forward

  1. I can publish a release containing your changes spotless-java7:1.3.0. It will be in a separate java7 branch. When/if people contribute things to the project, I'm not going to port them from Function<String, String> to FormattingOperation, but if you want to you're welcome to, and I'll publish them as your PR's arrive.

  2. You can redo the work so that it's a minimal-change port, rather than canonical Java 7. It will still be in a separate java7 branch, but I'd be happy to keep it up-to-date with changes contributed to the "canonical" java8 branch.

Basically, I feel really strongly that plugins should just be Function<String, String>, because it puts the least restrictions on implementors, and it doesn't require them to learn anything about Spotless to make a plugin. I also feel really strongly that the future of this plugin is Java 8 (because the future of the Eclipse formatter is Java 8), so I'm won't accept any design changes which are only driven by a 9-month-long Java 7 restriction.

If you disagree, it won't hurt my feelings if you decide you'd rather fork and release your own spotless-java7 or somenewproject which embraces a non-functional style. But I'm really convinced that replacing functions with classes that have init() methods is a step backwards.

@gaborbernat
Copy link
Author

Don't know of a port of the Function<String,String> for Java7 so can't really understand how to port, without making it canonical. Note it must compile under Java 7 to be Java 7 runnable.

The init function is automatically called, the user should not think about that.

@nedtwigg
Copy link
Member

This is the actual source code for Function from JDK 8:

public interface Function<T, R> {
    R apply(T t);
}

Most places in Spotless uses a variation of this which can throw exceptions. You can copy-paste this into the project in the same package: Throwing, and just remove the @FunctionalInterface annotations. A lot of the code would compile right off the bat.

The init function is automatically called, the user should not think about that.

If I'm making a plugin for Spotless, then I need to extend FormattingOperation. What should I put in the constructor, and what should I put in init()?

@nedtwigg
Copy link
Member

It seems that this partial implementation of Issue #7 has stalled out for now. If you still need this Gabor, feel free to reopen :)

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.

2 participants