-
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
Create a common interface for the license header #235
Create a common interface for the license header #235
Conversation
|
||
/** | ||
* | ||
*/ |
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.
Without seeing the issue that triggered this PR, it's not obvious what the point of this refactoring is. Maybe this would help?
Rename to HasBuiltinDelimiterForLicense
. Then you can add javadoc that says something like:
Every FormatExtension
has a method license(licenseContent, licenseDelimiter)
, where licenseDelimiter
is a regex that separates the license part of the code from the content. For some kinds of format - such as java, kotlin, and groovy - we already have a defined delimiter, so users don't have to provide it. By having the java, kotlin, and groovy formats implement this interface, you can write generic code for enforcing whitespace and licenses.
When the documentation for a feature takes more effort than working-around not having it, that's a code smell. It's a sign that most users are going to continue to work around it, because it takes less effort to workaround than to read and understand the docs to avoid the workaround.
If you still want to add it, I'll merge after you've renamed and improved docs. If you also now lean towards this being a little over-engineered, then that's good with me too :)
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.
Nah, I think your suggestion for documentation is right. I just totally dropped the ball. Also, I like the better name. Thanks!
@nedtwigg If you think this looks good now, let me know and I'll do the changelog change as well. |
LGTM, update changelog and I'll merge 👍 |
@nedtwigg Should be good to go. |
Closes #233
After creating the PR, please add a commit that adds a bullet-point under the
-SNAPSHOT
section of CHANGES.md and plugin-gradle/CHANGES.md which includes:If your change only affects a build plugin, and not the lib, then you only need to update the
CHANGES.md
for that plugin.If your change affects lib in an end-user-visible way (fixing a bug, updating a version) then you need to update
CHANGES.md
for both the lib and the build plugins. Users of a build plugin shouldn't have to refer to lib to see changes that affect them.This makes it easier for the maintainers to quickly release your changes :)