From e06e70eef4ae46288ed6f24f6b75dd5e3e787a38 Mon Sep 17 00:00:00 2001 From: Ned Twigg Date: Wed, 1 Jul 2020 22:05:51 -0700 Subject: [PATCH 1/7] Improve the parameter names in FormatterFunc. --- .../java/com/diffplug/spotless/FormatterFunc.java | 15 +++++++-------- .../diffplug/spotless/StepHarnessWithFile.java | 8 ++++---- 2 files changed, 11 insertions(+), 12 deletions(-) diff --git a/lib/src/main/java/com/diffplug/spotless/FormatterFunc.java b/lib/src/main/java/com/diffplug/spotless/FormatterFunc.java index ab4ca219b1..457b9b7e7c 100644 --- a/lib/src/main/java/com/diffplug/spotless/FormatterFunc.java +++ b/lib/src/main/java/com/diffplug/spotless/FormatterFunc.java @@ -1,5 +1,5 @@ /* - * Copyright 2016 DiffPlug + * Copyright 2016-2020 DiffPlug * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -27,8 +27,8 @@ public interface FormatterFunc extends ThrowingEx.Function, ThrowingEx.BiFunction { @Override - default String apply(String input, File source) throws Exception { - return apply(input); + default String apply(String unix, File file) throws Exception { + return apply(unix); } /** @@ -50,16 +50,15 @@ public void close() { } @Override - public String apply(String input, File source) throws Exception { - return function.apply(Objects.requireNonNull(input), Objects.requireNonNull(source)); + public String apply(String unix, File file) throws Exception { + return function.apply(Objects.requireNonNull(unix), Objects.requireNonNull(file)); } @Override - public String apply(String input) throws Exception { - return function.apply(Objects.requireNonNull(input)); + public String apply(String unix) throws Exception { + return function.apply(Objects.requireNonNull(unix)); } }; } } - } diff --git a/testlib/src/main/java/com/diffplug/spotless/StepHarnessWithFile.java b/testlib/src/main/java/com/diffplug/spotless/StepHarnessWithFile.java index df8de0fbe6..296ccadc11 100644 --- a/testlib/src/main/java/com/diffplug/spotless/StepHarnessWithFile.java +++ b/testlib/src/main/java/com/diffplug/spotless/StepHarnessWithFile.java @@ -44,13 +44,13 @@ public static StepHarnessWithFile forStep(FormatterStep step) { }, new FormatterFunc() { @Override - public String apply(String input) throws Exception { - return apply(input, new File("")); + public String apply(String unix) throws Exception { + return apply(unix, new File("")); } @Override - public String apply(String input, File source) throws Exception { - return LineEnding.toUnix(step.format(input, source)); + public String apply(String unix, File file) throws Exception { + return LineEnding.toUnix(step.format(unix, file)); } })); } From a084a18aaa478dcecdcca967b7d55475a6798ef3 Mon Sep 17 00:00:00 2001 From: Ned Twigg Date: Wed, 1 Jul 2020 22:18:14 -0700 Subject: [PATCH 2/7] Create FormatterFunc.NeedsFile --- .../com/diffplug/spotless/FormatterFunc.java | 30 +++++++++++++++++++ 1 file changed, 30 insertions(+) diff --git a/lib/src/main/java/com/diffplug/spotless/FormatterFunc.java b/lib/src/main/java/com/diffplug/spotless/FormatterFunc.java index 457b9b7e7c..fa8cdb54c7 100644 --- a/lib/src/main/java/com/diffplug/spotless/FormatterFunc.java +++ b/lib/src/main/java/com/diffplug/spotless/FormatterFunc.java @@ -23,6 +23,7 @@ * Also the `BiFunction` is supported, whereas the default * implementation only requires the `Function` implementation. */ +@FunctionalInterface public interface FormatterFunc extends ThrowingEx.Function, ThrowingEx.BiFunction { @@ -61,4 +62,33 @@ public String apply(String unix) throws Exception { }; } } + + /** + * Ideally, formatters don't need the underlying file. But in case they do, they should only use it's path, + * and should never read the content inside the file, because that breaks the `Function` composition + * that Spotless is based on. For the rare case that you need access to the file, use this method + * or {@link NeedsFile} to create a {@link FormatterFunc} which needs the File. + */ + static FormatterFunc needsFile(NeedsFile needsFile) { + return needsFile; + } + + /** @see FormatterFunc#needsFile(NeedsFile) */ + @FunctionalInterface + interface NeedsFile extends FormatterFunc { + String applyWithFile(String unix, File file) throws Exception; + + @Override + default String apply(String unix, File file) throws Exception { + if (file == null) { + throw new IllegalArgumentException("This step requires the underlying file. If this is a test, use StepHarnessWithFile"); + } + return applyWithFile(unix, file); + } + + @Override + default String apply(String unix) throws Exception { + return apply(unix, null); + } + } } From e6b9d9ce61e52bae4674229ffc04f56136d058fa Mon Sep 17 00:00:00 2001 From: Ned Twigg Date: Wed, 1 Jul 2020 22:18:30 -0700 Subject: [PATCH 3/7] Refactor our existing file-based steps in terms of it. --- .../extra/wtp/EclipseWtpFormatterStep.java | 16 +++----------- .../spotless/generic/LicenseHeaderStep.java | 22 ++----------------- .../spotless/npm/PrettierFormatterStep.java | 15 ++++--------- 3 files changed, 9 insertions(+), 44 deletions(-) diff --git a/lib-extra/src/main/java/com/diffplug/spotless/extra/wtp/EclipseWtpFormatterStep.java b/lib-extra/src/main/java/com/diffplug/spotless/extra/wtp/EclipseWtpFormatterStep.java index 22006a985a..63e8673774 100644 --- a/lib-extra/src/main/java/com/diffplug/spotless/extra/wtp/EclipseWtpFormatterStep.java +++ b/lib-extra/src/main/java/com/diffplug/spotless/extra/wtp/EclipseWtpFormatterStep.java @@ -15,7 +15,6 @@ */ package com.diffplug.spotless.extra.wtp; -import java.io.File; import java.lang.reflect.InvocationTargetException; import java.lang.reflect.Method; import java.util.Properties; @@ -71,13 +70,13 @@ private static FormatterFunc applyWithoutFile(String className, EclipseBasedStep }; } - private static FormatterFuncWithFile applyWithFile(String className, EclipseBasedStepBuilder.State state) throws Exception { + private static FormatterFunc.NeedsFile applyWithFile(String className, EclipseBasedStepBuilder.State state) throws Exception { Class formatterClazz = state.loadClass(FORMATTER_PACKAGE + className); Object formatter = formatterClazz.getConstructor(Properties.class).newInstance(state.getPreferences()); Method method = formatterClazz.getMethod(FORMATTER_METHOD, String.class, String.class); - return (input, source) -> { + return (unixString, file) -> { try { - return (String) method.invoke(formatter, input, source.getAbsolutePath()); + return (String) method.invoke(formatter, unixString, file.getAbsolutePath()); } catch (InvocationTargetException exceptionWrapper) { Throwable throwable = exceptionWrapper.getTargetException(); Exception exception = (throwable instanceof Exception) ? (Exception) throwable : null; @@ -85,13 +84,4 @@ private static FormatterFuncWithFile applyWithFile(String className, EclipseBase } }; } - - private static interface FormatterFuncWithFile extends FormatterFunc { - @Override - default String apply(String input) throws Exception { - throw new UnsupportedOperationException("Formatter requires file path of source."); - } - - public String apply(String input, File source) throws Exception; - } } diff --git a/lib/src/main/java/com/diffplug/spotless/generic/LicenseHeaderStep.java b/lib/src/main/java/com/diffplug/spotless/generic/LicenseHeaderStep.java index cd5ccebc91..59077f797e 100644 --- a/lib/src/main/java/com/diffplug/spotless/generic/LicenseHeaderStep.java +++ b/lib/src/main/java/com/diffplug/spotless/generic/LicenseHeaderStep.java @@ -89,30 +89,12 @@ public LicenseHeaderStep withYearModeLazy(Supplier yearMode) { return new LicenseHeaderStep(headerLazy, delimiter, yearSeparator, yearMode); } - private static class SetFromGitFormatterFunc implements FormatterFunc { - private final Runtime runtime; - - private SetFromGitFormatterFunc(Runtime runtime) { - this.runtime = runtime; - } - - @Override - public String apply(String input, File source) throws Exception { - return runtime.setLicenseHeaderYearsFromGitHistory(input, source); - } - - @Override - public String apply(String input) throws Exception { - throw new UnsupportedOperationException(); - } - } - public FormatterStep build() { if (yearMode.get() == YearMode.SET_FROM_GIT) { return FormatterStep.createNeverUpToDateLazy(LicenseHeaderStep.name(), () -> { boolean updateYear = false; // doesn't matter - Runtime step = new Runtime(headerLazy.get(), delimiter, yearSeparator, updateYear); - return new SetFromGitFormatterFunc(step); + Runtime runtime = new Runtime(headerLazy.get(), delimiter, yearSeparator, updateYear); + return FormatterFunc.needsFile(runtime::setLicenseHeaderYearsFromGitHistory); }); } else { return FormatterStep.createLazy(LicenseHeaderStep.name(), () -> { diff --git a/lib/src/main/java/com/diffplug/spotless/npm/PrettierFormatterStep.java b/lib/src/main/java/com/diffplug/spotless/npm/PrettierFormatterStep.java index b181fca796..565e93a26c 100644 --- a/lib/src/main/java/com/diffplug/spotless/npm/PrettierFormatterStep.java +++ b/lib/src/main/java/com/diffplug/spotless/npm/PrettierFormatterStep.java @@ -100,7 +100,7 @@ private void endServer(PrettierRestService restService, ServerProcessInfo restSe } - public static class PrettierFilePathPassingFormatterFunc implements FormatterFunc { + public static class PrettierFilePathPassingFormatterFunc implements FormatterFunc.NeedsFile { private final String prettierConfigOptions; private final PrettierRestService restService; @@ -110,16 +110,9 @@ public PrettierFilePathPassingFormatterFunc(String prettierConfigOptions, Pretti } @Override - public String apply(String input) throws Exception { - return apply(input, new File("")); - } - - @Override - public String apply(String input, File source) throws Exception { - requireNonNull(input, "input must not be null"); - requireNonNull(source, "source must not be null"); - final String prettierConfigOptionsWithFilepath = assertFilepathInConfigOptions(source); - return restService.format(input, prettierConfigOptionsWithFilepath); + public String applyWithFile(String unix, File file) throws Exception { + final String prettierConfigOptionsWithFilepath = assertFilepathInConfigOptions(file); + return restService.format(unix, prettierConfigOptionsWithFilepath); } private String assertFilepathInConfigOptions(File file) { From eb9265f9cb12a9fda0f202def2ba599cf3575571 Mon Sep 17 00:00:00 2001 From: Ned Twigg Date: Wed, 1 Jul 2020 22:19:24 -0700 Subject: [PATCH 4/7] Update changelog. --- CHANGES.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGES.md b/CHANGES.md index 82b6839819..2a52513606 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -16,6 +16,7 @@ We adhere to the [keepachangelog](https://keepachangelog.com/en/1.0.0/) format ( * `prettier` will now autodetect the parser (and formatter) to use based on the filename, unless you override this using `config` or `configFile` with the option `parser` or `filepath`. ([#620](https://github.com/diffplug/spotless/pull/620)) * `GitRatchet` now lives in `lib-extra`, and is shared across `plugin-gradle` and `plugin-maven` ([#626](https://github.com/diffplug/spotless/pull/626)). * Added ANTLR4 support ([#326](https://github.com/diffplug/spotless/issues/326)). +* `FormatterFunc.NeedsFile` for implementing file-based formatters more cleanly than we have so far ([#637](https://github.com/diffplug/spotless/issues/637)). ### Changed * **BREAKING** `FileSignature` can no longer sign folders, only files. Signatures are now based only on filename (not path), size, and a content hash. It throws an error if a signature is attempted on a folder or on multiple files with different paths but the same filename - it never breaks silently. This change does not break any of Spotless' internal logic, so it is unlikely to affect any of Spotless' consumers either. ([#571](https://github.com/diffplug/spotless/pull/571)) * This change allows the maven plugin to cache classloaders across subprojects when loading config resources from the classpath (fixes [#559](https://github.com/diffplug/spotless/issues/559)). From 0727a5ae12a63f79c669f826835a045f1ee4007b Mon Sep 17 00:00:00 2001 From: Ned Twigg Date: Wed, 1 Jul 2020 22:22:29 -0700 Subject: [PATCH 5/7] Hide some implementation details that leaked out on accident. --- .../java/com/diffplug/spotless/npm/PrettierFormatterStep.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/src/main/java/com/diffplug/spotless/npm/PrettierFormatterStep.java b/lib/src/main/java/com/diffplug/spotless/npm/PrettierFormatterStep.java index 565e93a26c..a7f95aa7c2 100644 --- a/lib/src/main/java/com/diffplug/spotless/npm/PrettierFormatterStep.java +++ b/lib/src/main/java/com/diffplug/spotless/npm/PrettierFormatterStep.java @@ -58,7 +58,7 @@ public static FormatterStep create(Map devDependencies, Provisio State::createFormatterFunc); } - public static class State extends NpmFormatterStepStateBase implements Serializable { + private static class State extends NpmFormatterStepStateBase implements Serializable { private static final long serialVersionUID = -539537027004745812L; private final PrettierConfig prettierConfig; @@ -100,7 +100,7 @@ private void endServer(PrettierRestService restService, ServerProcessInfo restSe } - public static class PrettierFilePathPassingFormatterFunc implements FormatterFunc.NeedsFile { + private static class PrettierFilePathPassingFormatterFunc implements FormatterFunc.NeedsFile { private final String prettierConfigOptions; private final PrettierRestService restService; From 3b13ae2f76840c0dfb9fa58815f73ed61fd9862a Mon Sep 17 00:00:00 2001 From: Ned Twigg Date: Wed, 1 Jul 2020 22:51:20 -0700 Subject: [PATCH 6/7] Fix spotbugs warning. --- .../java/com/diffplug/spotless/FormatterFunc.java | 13 +++++++------ .../com/diffplug/spotless/FormatterStepImpl.java | 5 ++++- 2 files changed, 11 insertions(+), 7 deletions(-) diff --git a/lib/src/main/java/com/diffplug/spotless/FormatterFunc.java b/lib/src/main/java/com/diffplug/spotless/FormatterFunc.java index fa8cdb54c7..f675896db9 100644 --- a/lib/src/main/java/com/diffplug/spotless/FormatterFunc.java +++ b/lib/src/main/java/com/diffplug/spotless/FormatterFunc.java @@ -19,15 +19,16 @@ import java.util.Objects; /** - * A `Function` which can throw an exception. - * Also the `BiFunction` is supported, whereas the default - * implementation only requires the `Function` implementation. + * A `Function` which can throw an exception. Technically, there + * is also a `File` argument which gets passed around as well, but that is invisible + * to formatters. If you need the File, see {@link NeedsFile}. */ @FunctionalInterface public interface FormatterFunc extends ThrowingEx.Function, ThrowingEx.BiFunction { - @Override + String apply(String input) throws Exception; + default String apply(String unix, File file) throws Exception { return apply(unix); } @@ -80,7 +81,7 @@ interface NeedsFile extends FormatterFunc { @Override default String apply(String unix, File file) throws Exception { - if (file == null) { + if (file == FormatterStepImpl.SENTINEL) { throw new IllegalArgumentException("This step requires the underlying file. If this is a test, use StepHarnessWithFile"); } return applyWithFile(unix, file); @@ -88,7 +89,7 @@ default String apply(String unix, File file) throws Exception { @Override default String apply(String unix) throws Exception { - return apply(unix, null); + return apply(unix, FormatterStepImpl.SENTINEL); } } } diff --git a/lib/src/main/java/com/diffplug/spotless/FormatterStepImpl.java b/lib/src/main/java/com/diffplug/spotless/FormatterStepImpl.java index 4f25d3c1e4..12c90b3031 100644 --- a/lib/src/main/java/com/diffplug/spotless/FormatterStepImpl.java +++ b/lib/src/main/java/com/diffplug/spotless/FormatterStepImpl.java @@ -1,5 +1,5 @@ /* - * Copyright 2016 DiffPlug + * Copyright 2016-2020 DiffPlug * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -110,4 +110,7 @@ protected String format(Integer state, String rawUnix, File file) throws Excepti return formatter.apply(rawUnix, file); } } + + /** A dummy SENTINEL file. */ + static final File SENTINEL = new File(""); } From f57e557d483634fff47970a253ae1cecffba2271 Mon Sep 17 00:00:00 2001 From: Ned Twigg Date: Wed, 1 Jul 2020 22:51:51 -0700 Subject: [PATCH 7/7] Improve CONTRIBUTING to point people who need the file here. --- CONTRIBUTING.md | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 7af707b059..8b3ef78542 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -44,7 +44,7 @@ For the folders below in monospace text, they are published on maven central at The easiest way to create a FormatterStep is `FormatterStep createNeverUpToDate(String name, FormatterFunc function)`, which you can use like this: ```java -FormatterStep identityStep = FormatterStep.createNeverUpToDate("identity", unix -> unix) +FormatterStep identityStep = FormatterStep.createNeverUpToDate("identity", unixStr -> unixStr) ``` This creates a step which will fail up-to-date checks (it is equal only to itself), and will use the function you passed in to do the formatting pass. @@ -73,7 +73,7 @@ public final class ReplaceStep { } FormatterFunc toFormatter() { - return raw -> raw.replace(target, replacement); + return unixStr -> unixStr.replace(target, replacement); } } } @@ -100,6 +100,10 @@ Here's a checklist for creating a new step for Spotless: - [ ] Test class has test methods to verify behavior. - [ ] Test class has a test method `equality()` which tests equality using `StepEqualityTester` (see existing methods for examples). +### Accessing the underlying File + +In order for Spotless' model to work, each step needs to look only at the `String` input, otherwise they cannot compose. However, there are some cases where the source `File` is useful, such as to look at the file extension. In this case, you can pass a `FormatterFunc.NeedsFile` instead of a `FormatterFunc`. This should only be used in [rare circumstances](https://github.com/diffplug/spotless/pull/637), be careful that you don't accidentally depend on the bytes inside of the `File`! + ## How to enable the _ext projects The `_ext` projects are disabled per default, since: