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

Refactored Eclipse formatter configuration and step creation #253

Merged
merged 18 commits into from
Jul 5, 2018
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -0,0 +1,257 @@
/*
* Copyright 2016 DiffPlug
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/
package com.diffplug.spotless.extra.config;

import java.io.File;
import java.io.IOException;
import java.io.Serializable;
import java.net.MalformedURLException;
import java.net.URL;
import java.nio.file.Path;
import java.nio.file.Paths;
import java.util.ArrayList;
import java.util.Objects;
import java.util.Properties;
import java.util.TreeSet;
import java.util.stream.Collectors;

import com.diffplug.spotless.FileSignature;
import com.diffplug.spotless.FormatterProperties;
import com.diffplug.spotless.JarState;
import com.diffplug.spotless.Provisioner;
import com.diffplug.spotless.ThrowingEx;

import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;

/**
* Eclipse formatter generic configuration validates the user arguments and creates on request
* a {@link State} of the current values.
*/
public class EclipseConfiguration implements ThrowingEx.Supplier<EclipseConfiguration.State> {
/**
* Resource location of Spotless Eclipse Formatter Maven coordinate lists.
* <p>
* Spotless Eclipse Formatter dependencies have fixed transitive versions, since Spotless Eclipse Formatter
* implementations access internal methods of the Eclipse plugins, which may change with every
* version change, including minor and patch version changes.
* At the resource location for each supported Spotless Eclipse Formatter, a text file is provided, containing
* the fixed versions for the formatter and its transitive dependencies.
* Each line in the text file corresponds to the {@link MavenCoordinates.Coordinate#FORMAT}.
* </p>
*/
private static final String ECLIPSE_FORMATTER_RESOURCES = EclipseConfiguration.class.getPackage().getName().replace('.', '/');

private final TreeSet<SemanticVersion> supportedEclipseVersions;
private final URL eclipseConfigContext;
private final Provisioner jarProvisioner;
private URL defaultCoordinatesUrl;
private MavenCoordinates coordinateAdaptations;
private SemanticVersion version;
private Iterable<File> settingsFiles;
private EclipseConfiguration.State state;

/** Initialize valid default configuration, taking latest version */
public EclipseConfiguration(String formatterName, Provisioner jarProvisioner, String... supportedEclipseVersions) {
Objects.requireNonNull(formatterName, "formatterName");
Objects.requireNonNull(jarProvisioner, "jarProvisioner");
Objects.requireNonNull(supportedEclipseVersions, "supportedEclipseVersions");
if (supportedEclipseVersions.length < 1) {
throw new IllegalArgumentException("At lease one version must be allowed.");
}

Path relativeFormatterResourcePath = Paths.get(ECLIPSE_FORMATTER_RESOURCES, formatterName.replace(' ', '_'));
eclipseConfigContext = getClass().getClassLoader().getResource(relativeFormatterResourcePath.toString());
if (null == eclipseConfigContext) {
throw new IllegalArgumentException(String.format("Eclipse configuration context resource path '%s' not found.", relativeFormatterResourcePath));
}

this.supportedEclipseVersions = new TreeSet<SemanticVersion>();
for (String allowedVersion : supportedEclipseVersions) {
this.supportedEclipseVersions.add(new SemanticVersion(allowedVersion));
}
version = this.supportedEclipseVersions.last(); //Use latest version per default
defaultCoordinatesUrl = getDefaultCoordinatesUrl(eclipseConfigContext, version);
this.jarProvisioner = jarProvisioner;

coordinateAdaptations = new MavenCoordinates(); //Use default coordinates configured for version
settingsFiles = new ArrayList<File>(); //Use default preferences
state = null; //Create state only on demand too speed up initialization
}

/** Set Eclipse version */
public void setVersion(String versionOrUrl) {
try {
// For development purpose and patch delivery it is allowed
// to specify directly the URL of a M2 coordinates file.
defaultCoordinatesUrl = new URL(versionOrUrl);
// The default version is not changed.
} catch (MalformedURLException ignored) {
SemanticVersion newVersion = new SemanticVersion(versionOrUrl);
if (supportedEclipseVersions.contains(newVersion)) {
defaultCoordinatesUrl = getDefaultCoordinatesUrl(eclipseConfigContext, newVersion);
version = newVersion;
} else {
throw new UserArgumentException(versionOrUrl,
String.format("Version is not part of the supported versions '%s'.",
supportedEclipseVersions.stream().map(v -> v.toString()).collect(Collectors.joining(", "))));
}
}
invalidateState();
}

private static URL getDefaultCoordinatesUrl(URL eclipseConfigContext, SemanticVersion version) {
try {
Path filePath = Paths.get(eclipseConfigContext.getPath(), String.format("v%s.txt", version.toString()));
return new URL(eclipseConfigContext.getProtocol(), eclipseConfigContext.getHost(), filePath.toString());
} catch (MalformedURLException e) {
/*
* This exception must be prevented by the strict syntax checking of
* SemanticVersion and unit-tests of all allowed versions.
*/
throw new RuntimeException(e);
Copy link
Member

Choose a reason for hiding this comment

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

I don't know @nedtwigg's stance on throwing plain RuntimeExceptions, but if it were me, I'd elect to throw a more specific exception: IllegalArgumentException if the user passed in an invalid URL or some other illegal argument, or IllegalStateException if it's either not the user's fault or is due to unexpected state in this. :)

}
}

/**
* Modify default Maven dependencies configured for the formatter.
* The default dependencies are located in {@link #ECLIPSE_FORMATTER_RESOURCES}.
*/
public void setDepenencies(String... coordinates) {
Copy link
Member

Choose a reason for hiding this comment

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

s/setDepenencies/setDependencies :)

coordinateAdaptations = new MavenCoordinates();
coordinateAdaptations.update(coordinates);
invalidateState();
}

/** Set settings files containing Eclipse preferences */
public void setPreferences(Iterable<File> settingsFiles) {
this.settingsFiles = settingsFiles;
invalidateState();
}

/** The current state becomes invalid in case a configuration item has successfully changed */
private void invalidateState() {
state = null;
}

/** Creates the state of the configuration */
public EclipseConfiguration.State get() {
if (null == state) {
state = new State(
jarProvisioner,
createMavenCoordinates(),
version,
settingsFiles);
}
return state;
Copy link
Member

Choose a reason for hiding this comment

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

I don't see an upside to caching the state. I would create a new state for every call to get().

Copy link
Member Author

Choose a reason for hiding this comment

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

Don't know who and how often get() is called. Since this code is not gradle specific, I wanted to be on the save side. I do access here files from the resources, so the procedure costs some time, though not very much. Anyhow, I have no strong feelings about this. Let me know you final decision.

Copy link
Member

Choose a reason for hiding this comment

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

There are 2 hard problems in computer science: cache invalidation, naming things, and off-by-1 errors.

:-P I think the safe thing is to avoid caching until it's absolutely necessary.

}

private MavenCoordinates createMavenCoordinates() {
MavenCoordinates coordinates = new MavenCoordinates();
coordinates.add(defaultCoordinatesUrl);
coordinates.update(coordinateAdaptations);
return coordinates;
}

/**
* State of Eclipse configuration items, providing functionality to derived information
* based on the state.
*/
public static class State implements Serializable {
// Not used, only the serialization output is required to determine whether the object has changed
private static final long serialVersionUID = 1L;

private final FileSignature settingsFiles;
private final MavenCoordinates coordinates;
private final SemanticVersion version;

/*
* Fields are transient because not needed to uniquely identify a Eclipse configuration state, and also because
* Gradle only needs this class to be serializable so it can compare its members for incremental builds.
*/
private transient ClassLoader lazyClassLoader;
@SuppressFBWarnings("SE_TRANSIENT_FIELD_NOT_RESTORED")
private final transient Provisioner jarProvisioner;
Copy link
Member

Choose a reason for hiding this comment

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

Normally, JarState is part of the serialized state:

State(String version, Provisioner provisioner, @Nullable File configFile) throws IOException {
this.jarState = JarState.from(MAVEN_COORDINATE + version, provisioner);
this.configSignature = FileSignature.signAsList(configFile == null ? Collections.emptySet() : Collections.singleton(configFile));
}
FormatterFunc createFormat() throws Exception {
ClassLoader classLoader = jarState.getClassLoader();

This allows you to remove coordinates and version from the state because their effects are captured by the JarState (you can keep them too if you need them elsewhere). JarState is more reliable, consider this:

  • repo1 has one set of artifacts
  • repo2 has the same names and versions, but the artifacts have been patched with different content

JarState captures this, because it hashes the content of the jars. The current approach would miss this.

Copy link
Member Author

@fvgh fvgh Jun 3, 2018

Choose a reason for hiding this comment

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

Since the resolution of the jars take time, I wanted to be sure that it's only done when necessary. So what I basically want to avoid is a download in the following scenario:

  1. Developer modifies code
  2. Modification is not valid (tests, findbugs,...)
  3. Formatter not executed due to prior errors

The download should not be triggered before it is assured that the Formatter will be executed. When I work on code I want to see quickly whether it works.

I am not sure when Gradle requests the state.

Copy link
Member

Choose a reason for hiding this comment

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

Spotless (gradle or maven) requests the state at the last possible moment - not until someone actually calls FormatterFunc::apply. Great care has been taken to put caching and lazy-evaluation all into the framework - adding second layers adds complexity without adding any performance.


/** State constructor expects that all passed items are not modified afterwards */
protected State(Provisioner jarProvisioner, MavenCoordinates coordinates, SemanticVersion version, Iterable<File> settingsFiles) {
this.jarProvisioner = jarProvisioner;
this.coordinates = coordinates;
this.version = version;
lazyClassLoader = null;

try {
this.settingsFiles = FileSignature.signAsList(settingsFiles);
} catch (NullPointerException e) {
throw new UserArgumentException(null, "Configuration file settings", e);
} catch (IOException e) {
throw new RuntimeException(e); //Canonical path problems are not user argument problems
}
}

Copy link
Member

Choose a reason for hiding this comment

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

the .equals() and .hashCode() methods don't matter, only the serialization is used to determine up-to-date-ness.

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 admit that I just added the functionality because it is demanded by the SerializableEqualityTester. Later I found that actually the FileSignature seems to have no test for serialization.
So I was not sure anymore what I really need and decided to wait for your review.
I really would like to have a test. Is there already a test utility similar to the SerializableEqualityTester omitting the hash test? Or shall I provide one?

Copy link
Member Author

Choose a reason for hiding this comment

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

The SemanticVersion shall provide three things:

  • Be convenient for the user so it treats for example 4.5 and 4.5.0 equally.
  • Check user input on a high level so that useful responses are provided. If later on some method reports that jars:/<some_path>/ has not been found, it is more difficult for the user to see that just his version configuration is a problem. This is especially a problem when just minor things, like a white space, are incorrect.
  • Allow simple comparison (less than, greater than) so that the Eclipse formatter step can determine whether it needs some different interface implementation.

Copy link
Member Author

@fvgh fvgh Jun 1, 2018

Choose a reason for hiding this comment

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

Beside the convenience / checks for user data, the MavenCoordinates shall

  • Distinguish whether default settings are added or versions are updated
  • Provide the coordinates as a set to the provisioner to allow transitive dependency mediation. To my understanding the concepts here in Maven and Gradle slightly differ. Keep in mind that the default versions are fixed, but I expect users (and me as a developer) to use version ranges to do some testings.

Copy link
Member

Choose a reason for hiding this comment

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

so it treats for example 4.5 and 4.5.0 equally.

This won't work as you intend for up-to-date checking. Up-to-date checking is based solely on the serialized representation, it doesn't care about .equals().

I added the functionality because it is demanded by the SerializableEqualityTester

SerializableEqualityTester is only meant to be used on either steps or the entire step state. Not the components of step state.

Up-to-date checking works like this:

  • every step is a subclass of this, which means that equals and hashCode are implemented in terms of the state's serialized representation
    abstract class Strict<State extends Serializable> extends LazyForwardingEquality<State> implements FormatterStep {
    private static final long serialVersionUID = 1L;
    /**
    * Implements the formatting function strictly in terms
    * of the input data and the result of {@link #calculateState()}.
    */
    protected abstract String format(State state, String rawUnix, File file) throws Exception;
    @Override
    public final String format(String rawUnix, File file) throws Exception {
    return format(state(), rawUnix, file);
    }
    }
  • if the state has sub-components (such as FileSignature or JarState), they get serialized into the state and compared there. FileSignature and JarState don't declare an equals or hashCode, because it would never run. Equality testing happens at the entire state level, not component-by-component.

SemanticVersion and MavenCoordinates

Provisioner only takes a List. So I don't see how we will pass this detailed semantic information to the step's implementation. We can always do feature testing a-la:

try {
// scalafmt >= v0.7.0-RC1
Method fromHocon = configCls.getMethod("fromHoconString", String.class, optionCls);
Object fromHoconEmptyPath = configCls.getMethod("fromHoconString$default$2").invoke(null);
String configStr = new String(Files.readAllBytes(file.toPath()), StandardCharsets.UTF_8);
Object configured = fromHocon.invoke(null, configStr, fromHoconEmptyPath);
either = invokeNoArg(configured, "toEither");
} catch (NoSuchMethodException e) {
// In case of a NoSuchMethodException try old configuration API
// scalafmt <= v0.6.8
Method fromHocon = configCls.getMethod("fromHocon", String.class, optionCls);
Object fromHoconEmptyPath = configCls.getMethod("fromHocon$default$2").invoke(null);
String configStr = new String(Files.readAllBytes(file.toPath()), StandardCharsets.UTF_8);
either = fromHocon.invoke(null, configStr, fromHoconEmptyPath);
}

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 am not sure what your goal is. I can reduce the LOC by integrating the functionality back into EclipseConfiguration, though I would still prefer to have it a separated inner class, to keep the code readable.
Your proposal still seem to assume that the classes are used externally, but they shall explicitly not, since this, as you pointed out and I a fully agree, would make of the developer harder. My goal for the Formatter Steps is something like this (the versionHigher does not exist yet):

class EclipseJdtFormatterStep .... {
...
private static String  INITV = "4.7.2"; //Highest version compatible to initial interface
private static VERSIONS[] = {"4.6.1", "4.6.3", "4.7.0", "4.7.1", INITV, "4.7.3"};
private final String implClassPath;

	public static FormatterStep createStep(EclipseConfiguration config) {
                String classPath = config.versionHigher(INITV)? "com.diffplug.spotless.extra.java.eclipse.EclipseJdtFormatterStepImpl" : ""com.diffplug.gradle.spotless.java.eclipse.EclipseFormatterStepImpl"
		return FormatterStep.createLazy(NAME, config, new EclipseJdtFormatterStep(classPath));
	}

}

The EclipseConfiguration and maybe later on some other XyzConfgiuration shall be the front end. The other package scope classes are just generic tools.

Copy link
Member Author

Choose a reason for hiding this comment

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

The tools have three goals:

  • Sanitize user input where convenient (3.4 -> 3.4.0, trim input, ...)
  • Validate user input if easily possible (for example the version range syntax is too complex, so I don't check it)
  • Provide functionality for the step configuration class to gain the information it needs (like comparison, initializing from specific file format, ...).

Copy link
Member

Choose a reason for hiding this comment

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

My goal is to minimize LOC. You've convinced me of Version, I'm not convinced about MavenCoordinates and Coordinate. I trust you, so we can leave them in if they help :)

Copy link
Member Author

Choose a reason for hiding this comment

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

One remaining question about the SerializableEqualityTester:
Do we have something like a SerializableTester? Do you think it is worth to have it, or are the compiler checks good enough?

Copy link
Member

Choose a reason for hiding this comment

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

I think the compiler check is good enough.

@Override
public boolean equals(Object obj) {
if (obj instanceof EclipseConfiguration.State) {
EclipseConfiguration.State other = (EclipseConfiguration.State) obj;
return this.hashCode() == other.hashCode();
}
return false;
}

@Override
public int hashCode() {
//Omit transient/derived information when providing the hash
return Objects.hash(settingsFiles, coordinates, version);
}

/**
* Comparison of configured Eclipse version with another version.
* Return one of -1, 0, or 1 according to whether the other version is higher, equal or lower.
*/
public int compareVersionTo(String otherVersion) {
return version.compareTo(new SemanticVersion(otherVersion));
}

/** Get formatter preferences */
public Properties getPreferences() {
//Keep the IllegalArgumentException since it contains detailed information
FormatterProperties preferences = FormatterProperties.from(settingsFiles.files());
return preferences.getProperties();
}

/** Load class based on the given configuration of JAR provider and Maven coordinates. */
public Class<?> loadClass(String name) {
try {
return getClassLoader().loadClass(name);
} catch (ClassNotFoundException e) {
throw new UserArgumentException(coordinates,
String.format("Could not find class '%s' in Maven coordinates.", name), e);
}
}

private ClassLoader getClassLoader() {
if (null == lazyClassLoader) {
String[] mavenCoordinates = coordinates.get();
try {
JarState jarState = JarState.from(jarProvisioner, mavenCoordinates);
lazyClassLoader = jarState.getClassLoader();
} catch (IOException e) {
throw new UserArgumentException(mavenCoordinates, "Not all dependencies have been resolved successfully.", e);
}
}
return lazyClassLoader;
}
}
}
Loading