Allow custom separator for years in license header by eyalkaspi · Pull Request #199 · diffplug/spotless

eyalkaspi · 2018-02-07T14:10:10Z

I've recently noticed the support for YEAR templates in license headers.

Unfortunately our code doesn't use the hyphen character but coma to separate year ranges (for example https://github.com/delphix/delphix-os/blob/a0f0d0dd2ed6832f0cca929a44b71bd8f33fee32/usr/src/uts/common/fs/zfs/dmu_zfetch.c#L27)

In this review, we are adding support in the library and gradle-plugin to configure the separator.

Instead of adding a third optional argument to the licenseHeader/licenseHeaderFile gradle configurations, we're creating a license header extension object which can be used as

format java {
  licenseHeader {
    header 'some-string'
    delimiter 'package'
    yearSeparator '-'
  }
}

However, both because most users won't need the advanced configuration and for backwards compatibility, we're maintaining support for the the existing syntax (the implementation always creates the extension, but that is transparent to users).

We've updated the Gradle plugin README.md to document and new feature and provide examples of both the simple (one line) and advanced (with licenseHeader extension) syntaxes.

We've updated the unit test for the license header step to cover the new feature, and updated the KotlinExtensionTest to cover the new gradle syntax (It appears that this is the only test covering the license header in the gradle plugin).

nedtwigg · 2018-02-07T14:20:59Z

Great, thanks! A few issues:

Are you able to get ./gradlew check to work before this commit?
If spotlessApply is removing annotations that's bad, but we should address it in a separate issue / PR
Rather than adding a LicenseHeaderExtension, how about following this pattern:

spotless/plugin-gradle/src/main/java/com/diffplug/gradle/spotless/ScalaExtension.java

Lines 38 to 65 in 115aa61

    
           public ScalaFmtConfig scalafmt() { 
        
           	return scalafmt(ScalaFmtStep.defaultVersion()); 
        
           } 
        
           public ScalaFmtConfig scalafmt(String version) { 
        
           	return new ScalaFmtConfig(version); 
        
           } 
        
           public class ScalaFmtConfig { 
        
           	final String version; 
        
           	@Nullable 
        
           	Object configFile; 
        
           	ScalaFmtConfig(String version) { 
        
           		this.version = Objects.requireNonNull(version); 
        
           		addStep(createStep()); 
        
           	} 
        
           	public void configFile(Object configFile) { 
        
           		this.configFile = Objects.requireNonNull(configFile); 
        
           		replaceStep(createStep()); 
        
           	} 
        
           	private FormatterStep createStep() { 
        
           		File resolvedConfigFile = configFile == null ? null : getProject().file(configFile); 
        
           		return ScalaFmtStep.create(version, GradleProvisioner.fromProject(getProject()), resolvedConfigFile); 
        
           	} 
        
           }

eyalkaspi · 2018-02-07T14:39:57Z

Thanks for the quick feedback.

Yes, ./gradlew check is working before my change, I introduced a bug in the first commit which is now fixed, sorry about that.

I don't mind using the pattern used for ScalaFmtConfig, but the difference here is that more than one property can be configured, so I don't think it could be applied directly. We could use a builder pattern, i.e

licenseHeader('header-string').delimiter('package').yearSeparator('-')

or allow multiple calls to update the same object

licenseHeader('header-string')
licenseHeader().delimiter('package')
licenseHeader().yearSeparator('-')

but none of these solutions seem idiomatic Gradle configurations (or maybe I misunderstood how the ScalaFmtConfig is being used)?

nedtwigg · 2018-02-07T15:04:14Z

ScalaFmtConfig is the builder pattern, exactly as your example. Here's another place it's used:

spotless/plugin-gradle/src/main/java/com/diffplug/gradle/spotless/JavaExtension.java

Lines 118 to 143 in 115aa61

    
           public class GoogleJavaFormatConfig { 
        
           	final String version; 
        
           	String style; 
        
           	GoogleJavaFormatConfig(String version) { 
        
           		this.version = Objects.requireNonNull(version); 
        
           		this.style = GoogleJavaFormatStep.defaultStyle(); 
        
           		addStep(createStep()); 
        
           	} 
        
           	public void style(String style) { 
        
           		this.style = Objects.requireNonNull(style); 
        
           		replaceStep(createStep()); 
        
           	} 
        
           	public void aosp() { 
        
           		style("AOSP"); 
        
           	} 
        
           	private FormatterStep createStep() { 
        
           		Project project = getProject(); 
        
           		return GoogleJavaFormatStep.create(version, 
        
           				style, 
        
           				GradleProvisioner.fromProject(project)); 
        
           	} 
        
           }

What's different about it compared to a regular builder is that the "build()" is implicit.

eyalkaspi · 2018-02-07T15:28:41Z

I think I understand what you mean, it's a builder in the sense that one can configure one property after having created the configuration object.

However, in order to really follow a builder or fluent style, individual methods of the config objects must return "this" so that calls can be chained (especially since a new Instance of the config object is created if the method if googleJavaFormat is called again).

Anyways, I've updated the PR to follow this style (Again, I would like to point out that this does not appear to be a very idiomatic way to do it in Gradle).

nedtwigg · 2018-02-07T15:34:23Z

Great! The steps that I linked have only one entry point and one optional second argument, so there's no need for chaining. Looks like the license header is gonna be growing as people need new features for it, so there'll be a need to return this for chaining as you noted.

nedtwigg · 2018-02-07T15:44:19Z

+		if (licenseHeaderConfig != null) {
+			licenseHeaderConfig.setupTask(this, task);
 		}
 		task.setLineEndingsPolicy(getLineEndings().createPolicy(getProject().getProjectDir(), () -> task.target));


Order matters for FormatterStep. e.g. if I have a license with trailing whitespace, I'll get a different result if I trim whitespace then apply the license, vs apply the license then trim whitespace. Currently, the order that you declare steps is the order that they all happen (which is the rationale behind the weird "replaceStep()" in the examples I linked. This approach allows you to not have a licenseHeaderConfig object.

After fixing this, just squash the commits and update the CHANGES.md and we should be good to go.

Ok, I've updated the PR to include the CHANGES and fully follow the existing "config object pattern" including the replaceStep() logic.

Again thanks for the feedback and very impressive response times.

nedtwigg

Found a few things I missed before. Once these are fixed I'll be ready to merge.

nedtwigg · 2018-02-08T14:55:48Z

+
+		/**
+		 * @param delimiter
+		 *            Spotless will look for a line that starts with this to know what the "top" is.


Spotless will look for the first match to this regex to what the "top" is.

See the kotlin separator for an example of why it has to be a regex.

Clarified the (pre-existing) comment

nedtwigg · 2018-02-08T14:56:26Z

+		String delimiter;
+
+		static final String DEFAULT_LICENSE_YEAR_DELIMITER = "-";
+		String yearSeparator = DEFAULT_LICENSE_YEAR_DELIMITER;


This default should be moved to LicenseHeaderStep.

Try to match this style:

spotless/lib/src/main/java/com/diffplug/spotless/java/GoogleJavaFormatStep.java

Lines 77 to 79 in 115aa61

public static String defaultVersion() {

return DEFAULT_VERSION;

}

nedtwigg · 2018-02-08T15:02:25Z

+	// plugin-gradle/src/main/java/com/diffplug/gradle/spotless/JavaExtension.java and TODO as well
 	private static final String LICENSE_HEADER_DELIMITER = "package ";
+	private static final String LICENSE_YEAR_DELIMITER = "-";



Rather than introducing changes to the tests, it would be better to use the backward-compatible LicenseHeaderStep. Not a big deal, fine to leave these as they are, just a tip for the future. When adding a feature, if you can add a default parameter and thus avoid changing a bunch of tests, just add a default parameter. The less things change, the easier it is for code-reviewers to verify that there aren't regressions.

nedtwigg · 2018-02-09T19:22:16Z

Can you click Allow edits from maintainers on this PR? I have some changes to add to your PR.

Allow edits from maintainers.

nedtwigg · 2018-02-09T22:16:13Z

Sorry, I screwed up the merge a bit. Anyway, it's merged now, but I made some changes first.

fa95810 - using nulls as default parameters is dangerous - just pass the default value
dd8e769 - let me know if you'd like to be attributed under a different name or different url

Thanks for the feature!

jbduncan · 2018-02-09T22:35:19Z

using nulls as default parameters is dangerous - just pass the default value

I very, very much agree with this. I actually spotted the corresponding piece of code a couple of hours or so ago, so thanks for addressing it for me @nedtwigg! :)

nedtwigg · 2020-06-05T04:33:47Z

If you care about license headers, you should checkout our new ratchetFrom 'origin/master' feature.

nedtwigg reviewed Feb 7, 2018

View reviewed changes

eyalkaspi force-pushed the master branch from bd38e49 to 7927e7e Compare February 7, 2018 19:57

nedtwigg requested changes Feb 8, 2018

View reviewed changes

Allow custom separator for years in license header

8756832

eyalkaspi force-pushed the master branch from 7927e7e to 8756832 Compare February 9, 2018 11:04

nedtwigg merged commit 8756832 into diffplug:master Feb 9, 2018

nedtwigg added a commit that referenced this pull request Feb 9, 2018

Merge pull request #199 from eyalkaspi/master

2a8260d

	public static String defaultVersion() {
	return DEFAULT_VERSION;
	}

Conversation

eyalkaspi commented Feb 7, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nedtwigg commented Feb 7, 2018

Uh oh!

eyalkaspi commented Feb 7, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nedtwigg commented Feb 7, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

eyalkaspi commented Feb 7, 2018

Uh oh!

nedtwigg commented Feb 7, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nedtwigg left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nedtwigg commented Feb 9, 2018

Uh oh!

nedtwigg commented Feb 9, 2018

Uh oh!

jbduncan commented Feb 9, 2018

Uh oh!

nedtwigg commented Jun 5, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

eyalkaspi commented Feb 7, 2018 •

edited

Loading

eyalkaspi commented Feb 7, 2018 •

edited

Loading

nedtwigg commented Feb 7, 2018 •

edited

Loading