Common filter for files not supported by LicenseHeaderStep #285
Conversation
| public static final SerializableFileFilter LISENCE_HEADER_UNSUPPORTED_FILES_FILTER = SerializableFileFilter.skipFilesNamed( | ||
| "package-info.java", "package-info.groovy", "module-info.java"); | ||
|
|
||
| private GenericConstants() {} |
There was a problem hiding this comment.
LISENCEought to be spelledLICENSE- This ought to live in
LicenseHeaderStepasJAVA_FILES_SKIPor something like that - It should be exposed as a
public static SerializableFileFilter method();rather thanpublic static final SerializableFileFilter field;. Changing the value of a public compile-time constant breaks binary compatibility.
There was a problem hiding this comment.
Should be fixed now. The field is called UNSUPPORTED_FILES_FILTER and method that exposes it is LicenseHeaderStep#unsupportedFilesFilter(). Wdyt?
Would such added public static final field become part of the public API? Who is the consumer of this API?
There was a problem hiding this comment.
Would such added public static final field become part of the public API?
Yup, if anyone else compiles code against this, the value of the constant gets compiled into their code. If we change the value of the constant, their code won't change. Putting it behind a method call fixes this.
Who is the consumer of this API?
Nobody that I know of. We can be pretty cavalier about breaking ABI and we'd get away with it, but when it's easy to keep ABI then we might as well.
unsupportedFilesFilter()
The trouble is that LicenseHeaderStep is used for lots of different languages, and this constant is Java-specific. If we need a different filter for Kotlin or Scala or something, how would we name it? How about unsupportedJavaFilesFilter()?
There was a problem hiding this comment.
I get the ABI point now. Thanks for the explanation.
Filter also excludes package-info.groovy that is why I tried not to mention Java in the method name :) Maybe it could be a single filter of all unsupported files?
There was a problem hiding this comment.
How about unsupportedJvmFilesFilter()? We've open PR's for c/c++ and typescript formatters. It's easier to build it modular now, and have the option to return the same filter for each method, rather than to build it coupled, and have to deprecate later if we need to uncouple.
There was a problem hiding this comment.
unsupportedJvmFilesFilter() looks good to me. Updated the commit.
lutovich
force-pushed
the
common-file-filter
branch
from
5fd638c to
54121b1
Compare
Extracted the filter into a shared constant which is used by all Gradle extensions and Maven mojo.
lutovich
force-pushed
the
common-file-filter
branch
from
54121b1 to
5cd1c06
Compare
|
Looks great! Sorry to nitpick so much for a small change :) |
|
Thank you for taking care of this!! |
3 participants
Extracted the filter into a shared constant which is used by all Gradle extensions and Maven mojo.
Resolves #270