-
Notifications
You must be signed in to change notification settings - Fork 461
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
Conversation
…rt dependency version locks and version dependent implementation interfaces. Dependency version locks are required in case Eclipse formatters dependencies are (partly) no longer part of a single fat JAR. Support of version dependent interface implementation shall ensure flexibility for future interfaces changes (renaming, adding logging options, ...). As part of the refactoring, user data checks have been added for Eclipse formatter configurations. Added support of user dependency modifications of Eclipse formatters. Changed JarState to cope with multiple coordinates. Added for FileSignature to provide comparison via hash.
The PR is not complete. Currently there are still the following tasks open:
Before I continue, I would prefer that we have an agreement on the interface changes and the location and scope of the new functionality. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't complete the review 100%, but I see two main issues:
First thing is that Spotless' up-to-date checking is always based only on the serialized representation being identical: https://github.com/diffplug/spotless/blob/master/lib/src/main/java/com/diffplug/spotless/LazyForwardingEquality.java
equals()
and hashCode()
don't matter - they won't be respected if the serialized representation changes, so I think implementing them is a bad idea. Skipping them makes the code a good bit smaller too.
The second thing is that gradle 4.8 has dependency locking built-in: https://docs.gradle.org/4.8-rc-3/userguide/dependency_locking.html
I think it's okay for us to have our own little system (we need to support maven too after all), but I wonder if we can get by with something simpler. For example, rather than SemanticVersion and parsing coordinates ourselves, can we just have a TreeSet<String>
, and pass each String to the Provisioner for parsing? Provisioner only takes strings in the end anyway, so what do we gain by modeling the coordinates and version ranges ourselves? Removing them would cut the LOC and long-term maintenance of this PR by quite a bit.
throw new RuntimeException(e); //Canonical path problems are not user argument problems | ||
} | ||
} | ||
|
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
and4.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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
spotless/lib/src/main/java/com/diffplug/spotless/FormatterStep.java
Lines 65 to 78 in c6c32e3
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:
spotless/lib/src/main/java/com/diffplug/spotless/scala/ScalaFmtStep.java
Lines 98 to 115 in c6c32e3
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); | |
} |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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, ...).
There was a problem hiding this comment.
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 :)
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
version, | ||
settingsFiles); | ||
} | ||
return state; |
There was a problem hiding this comment.
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().
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
|
||
// Not used, only the serialization output is required to determine whether the object has changed | ||
private static final long serialVersionUID = 1L; | ||
private final Set<Coordinate> coordinates; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's make this TreeSet<Coordinate>
to make it obvious that this field is sorted for deterministic serialization.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually I am not that sure whether they should be sorted. At least in Maven I thought that in the past the order of dependencies had an influence on determining the versions of the transitive dependencies. Since you pointed that line out, I just remember that I wanted to ask you, whether you know the details. In the Eclipse use case it probably will probably never matter...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know if it matters either :) If you prefer, it could also be List
. Because up-to-date is based on serialized, a regular Set
is unreliable. If the order changes, it will never be up-to-date, and Set
rehashing can cause the order to change. List
doesn't guarantee non-duplicates, but that's not the end of the world.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As I understand this article Maven uses a "nearest definition" principal, meaning the transitive dependencies are taken into account after the direct once.
Since the default dependencies are resolved and the depth information is removed by Gradle lock, the user anyhow has to specify each dependency which needs to be overridden. Hence we get the same results for Gradle and Maven users.
Using a flat fixed list of deps is anyway the goal of this PR, regardless whether we use Gradle lock to obtain it.
So I would stick with your initial request.
Let's make this TreeSet
I will add some documentation to the code summarizing the principal and justifying the tree. OK?
this.coordinates.add(coordinate); | ||
} | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Gradle up-to-date checking is just based on serialization, no need to implement equals or hashCode. I think dangerous to implement because they aren't actually used / respected by the up-to-date check.
public String toString() { | ||
return String.format("%s:%s", getDependency(), getVersionRange()); | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same up-to-date problem as earlier.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you wanted to refer to the hashCode
. The toString
is just for readable exception messages.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct! I expected it to highlight above my click rather than below, apologies :)
/** | ||
* Exceptions caused due to invalid user arguments. | ||
*/ | ||
class UserArgumentException extends RuntimeException { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we just use IllegalArgumentException
and Preconditions
class? I don't see why they can't fulfill the needs of this class.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This shall just ensure that the error message contains a value which can be recognized by the user, so that the user knows where something needs to be fixed. Furthermore it allows distinction between user config errors and internal errors.
Thought it would be a neat idea, but I can remove it if you like. Let me know your decision.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I prefer to be ruthless in minimizing the amount of code we have to maintain, and the amount of code others have to read to contribute. I'd prefer to remove it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wow, this is quite a big change! I don't fully understand the reason for this PR, but despite that I thought I'd give it a scan to see if there are any small parts which could be improved.
@fvgh Let me know what you think. :)
* Modify default Maven dependencies configured for the formatter. | ||
* The default dependencies are located in {@link #ECLIPSE_FORMATTER_RESOURCES}. | ||
*/ | ||
public void setDepenencies(String... coordinates) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/setDepenencies/setDependencies :)
} | ||
|
||
/** Get all dependency restrictions */ | ||
String[] get() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Effective Java says to prefer lists over arrays, unless e.g. the performance gained from using an array is measurably significant and truly needed.
So unless you have numbers from a JMH benchmark to prove that we need to return String[]
here, I would heartily recommend returning an unmodifiable List<String>
instead, e.g.:
List<String> get() {
return coordinates
.stream()
.map(c -> c.toString())
.collect(toCollectionAndThen(ArrayList::new, Collections::unmodifiableList));
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I prefer to keep the get/add/update interfaces aligned.
The only user for get
(beside tests) is the JarState
. As part of my changes, I also refactored it to support "varargs". It did not support lists before and I thought it would be a nice way to avoid dedicated methods for a single object and a collection.
I leave the choice to you guys whether we should avoid "varargs" and provide distinct methods for single object and set or collection.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The only user for get (beside tests) is the JarState.
Okay, fair point! But I personally prefer returning an unmodifiable collection because IMO it keeps things conceptually simpler. :)
As part of my changes, I also refactored it to support "varargs". It did not support lists before and I thought it would be a nice way to avoid dedicated methods for a single object and a collection.
I admit I'm not such a fan that the varargs forced the "mavenCoordinates" parameter to be moved to the end of the parameter list of JarState.from()
in this case.
So to reduce potential confusion for new people reading over JarState's constructors and JarState.from()
, I would revert this refactoring and introduce a new JarState.from(Iterable<String> mavenCoordinates, Provisioner provisioner)
static factory method instead. :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would also eliminate the need to @Deprecate
JarState.from(String, Provisioner)
, AFAICT.
this.coordinates.addAll(additionalCoordinates); | ||
} finally { | ||
reader.close(); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please use try-with-resources over the old try-finally-close pattern. :)
throw new UserArgumentException(null, "Maven coordinate"); | ||
} | ||
LinkedList<String> dependencyParts = new LinkedList<String>( | ||
Arrays.asList(coordinate.split(":", -1))); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Prefer ArrayList
to LinkedList
, unless you have a JMH benchmark which proves that LinkedList
actually performs better.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👎
ArrayList does not support pollLast/pollFirst.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, in that case, try Deque
and ArrayDeque
instead. For queue- and stack-like operations like pollFirst
/pollLast
, ArrayDeque is nearly always the best implementation to use. To quote the ArrayDeque class javadoc:
This class is likely to be faster than Stack when used as a stack, and faster than LinkedList when used as a queue.
throw new UserArgumentException(null, "Version information"); | ||
} | ||
LinkedList<String> versionParts = new LinkedList<String>( | ||
Arrays.asList(version.split("\\.", -1))); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ditto wrt. LinkedList
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👎
I use addLast here. Performance does not matter. I just find the code neater. But let me know if you have a more elegant way to fill up missing minor/patch entries.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In that case, try Deque
and ArrayDeque
instead. :)
|
||
@Override | ||
public int hashCode() { | ||
return Objects.hash(version[MAJOR], version[MINOR], version[PATCH]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe this can be shortened to Arrays.hashCode(version)
.
* This exception must be prevented by the strict syntax checking of | ||
* SemanticVersion and unit-tests of all allowed versions. | ||
*/ | ||
throw new RuntimeException(e); |
There was a problem hiding this comment.
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 RuntimeException
s, 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
. :)
…dep_config # Conflicts: # plugin-gradle/src/main/java/com/diffplug/gradle/spotless/JavaExtension.java
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Your grand plan is coming into view!! I have an idea that I think can make this easier to read and use, let me know when you're at a stopping point and I'll commit my proposal.
I'd like to rename EclipseConfiguration
to EclipseBasedStepBuilder
. There are a few wrinkles that can hide more of the plumbing from the user that are hard to explain, so just lemme know when I can push in a commit without causing conflicts for you :)
*/ | ||
private transient ClassLoader lazyClassLoader; | ||
@SuppressFBWarnings("SE_TRANSIENT_FIELD_NOT_RESTORED") | ||
private final transient Provisioner jarProvisioner; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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:
- Developer modifies code
- Modification is not valid (tests, findbugs,...)
- 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.
There was a problem hiding this comment.
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.
public static String defaultVersion() { | ||
return DEFAULT_VERSION; | ||
return "2.3.0"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe VERSIONS[0]
or VERSIONS[VERSIONS.length-1]
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Currently the SemanticVersions are sorted by the EclipseConfiguration/EclipseBasedStepBuilder. I did not want to implicitly force the step implementation to do that. The return value "2.3.0" is in a way correct. The method is deprecated. You get the version you always got with prior versions for extra-lib.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
|
||
GrEclipseConfig(String version, FormatExtension extension) { | ||
GrEclipseConfig(@Nullable String version, FormatExtension extension) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
null parameters that get replaced with defaults are more fragile than explicitly passing default values. Consider:
public static final String DEFAULT_VERSION = "1";
public static int computeFromVersion(String version) {
return version.hashCode()
}
public static void main(String[] args) {
computeFromVersion(DEFAULT_VERSION);
}
vs
public static final String DEFAULT_VERSION = "1";
public static int computeFromVersion(@Nullable String version) {
if (version == null) {
version = DEFAULT_VERSION;
}
return version.hashCode()
}
public static void main(String[] args) {
computeFromVersion(null);
}
The null
case adds 3 lines, and what version ends up being is invisible to the user - they have to know the guts of the function to guess what might happen. It also means that computeFromVersion
is no longer a pure function of its inputs, it is now a function of its input and some hidden default state.
It's a small change, but multiplied across many lines and many programmers, null as default is costly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
First of all, the constructor is package protected and not used by the user directly.
The EclipseConfiguration/EclipseBasedStepBuilder should be initialized by the step and prevent an invalid state.
What about
GrEclipseConfig(FormatExtension extension) {
this.extension = extension;
config = GrEclipseFormatterStep.createConfig(GradleProvisioner.fromProject(extension.getProject()));
extension.addStep(GrEclipseFormatterStep.createStep(config));
}
GrEclipseConfig(String version, FormatExtension extension)
{
this(extension);
config.setVersion(version);
}
The drawback here is that in the second constructor extension.replaceStep is required. But I find this concept anyway annoying. Gradle and Maven are both state machines ensuring that the execution is triggered after the configuration has been completed. Hence I would prefer to pass a factory method to FormatterStep. But I wanted to keep such things out of this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You've done a ton of work, and you're getting lots of style nitpicks which must be frustrating :) Let me know when you're at a stopping point, so I can make some cleanup commits. Might be easier to just show what I mean. You'll be free to revert whichever of the commits you don't like.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You've done a ton of work, and you're getting lots of style nitpicks which must be frustrating :)
@fvgh Indeed, so apologies for only adding to the potential frustration!
I'm impressed by the amount of work you've put into this PR, so please keep up the good work. I'll do my best to point out the good things you do as well in the future, rather than just focusing on what could be improved. :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I was short at time yesterday (and for the rest of the week). I think my formulations were a little bit harsh. That was not intended. I appreciate what I can learn from you guys. Feel free to take over.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You weren't being harsh, I just know how I feel when my PR's get picked apart :) I pushed up four commits. Revert any that you don't think are helpful. I see a few remaining problems:
- Overloading
setVersion
to mean eithersetVersion
orsetHotfixCoordinatesUrl
is dangerous, better to split them up - If we split it up, then we don't need to pass in the
supportedEclipseVersions
, we could just throw "version '123' is unsupported" if we don't find the resource - The null default version is still dangerous, the steps should define a default version that we pass explicitly
- The resource handling code doesn't run on Windows, and 15 minutes of fiddling didn't fix it
I think fixing resource handling on Windows will be easier after the first three checkboxes are ticked.
As an aside, I'm less convinced than ever about SemanticVersion
and MavenCoordinates
vs List<String>
, but I'll let it be since it's an implementation detail that we can fix later, and I might not be understanding the full picture :)
@@ -145,34 +150,38 @@ private FormatterStep createStep() { | |||
} | |||
|
|||
public EclipseConfig eclipse() { | |||
return eclipse(EclipseFormatterStep.defaultVersion()); | |||
return new EclipseConfig(null); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
null as default = bugs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See previous comment.
I call it a day anyhow. Feel free to make changes. In the end we do not have to agree on everything. I explained the reasons for my implementation choices, but I trust your decisions more than mine. Just a few things I really want to avoid, since I think it did not serve us well in the past:
An extra POJO like config-class means always more LOC, but I think it is much better maintainable and also easy to use. |
…int of "ThrowingEx.Supplier" instead of plain-old "Supplier"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi!
I do not have a complete understanding of the eclipse formatter infrastructure and thus can't really say much about the concept. Made only a couple tiny comments about the code and docs.
Couple (possibly silly) questions come to my mind after reading the PR:
- Why doesn't spotless depend on a fixed version of the eclipse formatter?
- Isn't it a lot of complexity for the user to configure formatter version and multiple dependencies? (maybe complexity is optional and thus it is not an issue)
- When would user need / want to configure version and dependencies with version ranges?
Answers and insights would be greatly appreciated :)
// Not used, only the serialization output is required to determine whether the object has changed | ||
private static final long serialVersionUID = 1L; | ||
|
||
private final int[] version; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Imho 3 separate int
fields would represent the state in this class a bit cleaner
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't know, had it in the beginning like that and found the code ugly. Now I find it less ugly 😉. In the end a semantic version always depends on three digits, which needs to be processed in order. This is a principal used in many places. For readability, this principal could be sub-classed, where the sub-class has the three values as separated members but provide also list functionality. But I found that a little bit too verbose.
SemanticVersion previous = new SemanticVersion("0"); | ||
Arrays.asList("1", "1.1", "1.1.1", "1.1.2").forEach(ascending -> { | ||
SemanticVersion current = new SemanticVersion(ascending); | ||
assertThat(current.compareTo(current)).isZero(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe use #isEqualByComparingTo()
, #isLessThan()
and #isGreaterThan()
for these 3 assertions
plugin-maven/README.md
Outdated
<eclipse> | ||
<file>${basedir}/eclipse-fmt.xml</file> | ||
<dependencies> | ||
<param>org.eclipse.jdt:org.eclipse.jdt.core:3.13.2</param> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can it be <dependency>...</dependency>
instead of <param>...</param>
? "Dependency" would read a bit nicer
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Honestly, I hoped that you can tell me how Maven Plugin treats Array array parameters.
The code is:
@Parameter
private String[] dependencies;
Can I just use any XML element name for the single values I like? Did not find this obvious in the Mavan documentation, but I had no time to read it in detail. I would very much appreciate if you could change the Eclipse.java
so that it works.
BTW: I removed the required = true
from the file
parameter. From Eclipse formatter / configuration side, you don't have to specify it, but I am not sure whether we already discussed that and you found it cleaner to make it as required. Can you remind me?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think maven does not really care about names of the nested tags :) My comment was just about the README file. Doc I used for the maven plugin is here. Quick local experiment shows that all the following configs result in [1, 2, 3]
:
<dependencies>
<dependency>1</dependency>
<dependency>2</dependency>
<dependency>3</dependency>
</dependencies>
<dependencies>
<param>1</param>
<param>2</param>
<param>3</param>
</dependencies>
<dependencies>
<dependency>1</dependency>
<param>2</param>
<foo>3</foo>
</dependencies>
We have not discussed the file
parameter. It is definitely okay for it to be optional if Eclipse formatter can handle this.
@lutovich
I see no other way than to use Eclipse internal functionality. Hence the version ranges defined in the Eclipse plugins for their transitives don't protect my code, since these ranges are only valid if you stick to the API.
I just wanted this feature to have a transparent way for the user to figure out, whether a new Eclipse plugin version really solves their issues. I am just afraid that too many users asking to provide new versions, though the new Eclipse version does not fix their problems. It is also for us a possibly to ask the user to test quickly whether a new version would help them. But maybe it is too verbose and complex and should not be inserted before there is really the demand. |
…aller the code gets. I don't have the full picture, so feel free to revert this if I'm missing something. But it seems like we can implement the same functionality with less maintenance burden.
Let me know what you think of this version of the code, @fvgh. Really do feel free to revert it, I just wanted to show what I meant by the dependencies just being a I promise not to much with the code again for a while :) |
@nedtwigg Sorry for the delay. Lets have a look at the major goals of this PR: Provision of dependency version locks for Eclipse formatters 👍User dependency modifications 👎I see that you removed the possibility for the user to overwrite particular dependencies and user input checks. I don't think that in this way we should allow the user to change dependencies. As I explained in #226, we are talking about 35 dependencies. But as mentioned before, we can leave out this option until there is really the demand. ProposalI remove the remains of the configuration option from Flexibility for future interfaces changes / version dependent interface implementation 👎The ProposalI assume that you still want to overcome this problem by feature testing as you mentioned before. So be it. I will revert some of the obsolete changes I made and fix some of the Since feature testing is always quite ugly, do you think the artifact ID change in #239 shall remain? |
I don't know what you mean, but remove and revert at will! There are lots of potential use-cases that you have in mind, and I think your first draft tried to handle a lot of them in one class. My revisions were an attempt to strip that down to handling only the core use-case, with the hope of constructing the other use-cases out of the pieces of the core
You still have it, in the form of public static SemanticVersion parseVersion(List<String> dependencies, String groupId, String artifactId) { ... }
// in use
if (SemanticVersion.parseVersion(dependencies, "org.eclipse.jdt", "org.eclipse.jdt").isLessThan("1.5.0")) { Having a very simple, easy-to-understand state is a big advantage imo. You can easily parse out the information you want. But by keeping that parsing out of the state, you simplify what it takes to understand the state. This kind of SemanticVersion has lower requirements than the previous one - it doesn't need serializable, equals, or hashCode. It just needs convenience methods to do whatever version checks you need. It also doesn't need to be built until we actually have a version test that we want to do. This is an example of handling use-cases as independent components, rather than baking all of the details directly into EclipseBasedStepBuilder. |
…f Eclipse formatter. Removed config package.
@nedtwigg Please have a look at my commits. Main purpose:
I also remove the |
Both new commits (and the PR as a whole) LGTM. Feel free to merge if you feel ready :) |
Refactored Eclipse formatter configuration and step creation to support dependency version locks and version dependent implementation interfaces.
Dependency version locks are required in case Eclipse formatters dependencies are (partly) no longer part of a single fat JAR.
Support of version dependent interface implementation shall ensure flexibility for future interfaces changes (renaming, adding logging options, ...).
As part of the refactoring, user data checks have been added for Eclipse formatter configurations.
Added support of user dependency modifications of Eclipse formatters.
Changed JarState to cope with multiple coordinates.
Added hash comparison to FileSignature.