Allow custom separator for years in license header

[eyalkaspi]

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]

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]

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]

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]

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]

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]

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.

[eyalkaspi]

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]

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.

[eyalkaspi]

Clarified the (pre-existing) comment

[nedtwigg]

This default should be moved to LicenseHeaderStep.

[nedtwigg]

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;
	}

[eyalkaspi]

done

[nedtwigg]

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.

[eyalkaspi]

done

[nedtwigg]

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

Allow edits from maintainers.

[nedtwigg]

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]

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]

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