add a $YEAR as a supported variable in license #179
Conversation
This token is replaced by the current year in the following cases: * the license is missing * the license in not formatted * the $YEAR is not like 1990 or 1990-2005
e80f34d
to
6a41930
Compare
|
Great PR! This is a messy topic, but your solution handles it in a way that is easy to understand and document, but actually handles the fact that time is rolling forward, unlike our current solution which forces users to do a once-a-year format-every-file. I really like it, but I'd like to have at least one more Spotless contributor take a look before we merge and release. @jbduncan, @fvgh, can one/both of you take a look? |
|
What about supporting a force update mode so that you can optionally force all of your years to get updated to the current date? Something like |
|
We already support forcing all years to be the current date, and this change leaves that feature intact. If we can cover all usecases with fewer features, that will be easier to maintain and document. |
|
Maybe I misunderstand. How? Oh, are you saying by changing the Licence file to be that date running spotless then replacing it with |
|
|
||
| StepHarness.forStep(step) | ||
| .test(getTestResource(KEY_FILE_WITHOUT_LICENSE), getFileContentWithYEAR(currentYear())) | ||
| .testUnaffected(getFileContentWithYEAR(currentYear())) |
There was a problem hiding this comment.
From a reproducibility standpoint, using the currentYear() instead of a set year like "2003" seems a bit bad to me. Because of this and how there is already a test for "2003", it's not clear to me that testing against currentYear() adds any value.
There was a problem hiding this comment.
Since this token is not something configurable, Spotless will always replace the license with YearMonth.now(), so in test its difficult to put here a fixed value.
I see your point on test reproducibility.
If we want to do that, we would need to have more configuration on the license header step:
- token name (optional) -> default
$YEAR - closure with the replacement for the token -> default
YearMonth.now.().getYear() - the regex to check if the current content of this token is ok (optional) -> default
[0-9]{4}(-[0-9]{4})?
with that, we could set the configuration in test to fixed values instead of a moving one like YearMonth.now.().getYear()
| .test(getFileContentWithYEAR("not a year"), getFileContentWithYEAR(currentYear())); | ||
| } | ||
|
|
||
| private String getFileContentWithYEAR(String year) throws IOException { |
There was a problem hiding this comment.
I think it makes more sense for the "YEAR" in this method name to be replaced with "YearToken", to be consistent with LicenseHeaderStep.java and Java practices in general.
There was a problem hiding this comment.
renamed. but its more really the content of the $YEAR than the token itself
| private static final String KEY_FILE_WITHOUT_LICENSE = "license/MissLicenseWithYear.test"; | ||
| private static final String KEY_FILE_WITH_PREVIOUS_YEAR = "license/LicenseWithPreviousYear.test"; | ||
| private static final String KEY_FILE_WITH_PREVIOUS_YEARS = "license/LicenseWithPreviousYears.test"; | ||
| private static final String KEY_FILE_WITH_CURRENT_YEAR = "license/LicenseWithYear.test"; |
There was a problem hiding this comment.
I believe that KEY_FILE_WITH_PREVIOUS_YEAR and KEY_FILE_WITH_PREVIOUS_YEARS aren't used. If they are indeed unused, and if there are any other unused constants here that I've missed, then I think they should be removed.
|
|
||
| public class LicenseHeaderStepTest extends ResourceHarness { | ||
| private static final String KEY_LICENSE = "license/TestLicense"; | ||
| private static final String KEY_FILE_NOTAPPLIED = "license/MissingLicense.test"; | ||
| private static final String KEY_FILE_APPLIED = "license/HasLicense.test"; | ||
| private static final String KEY_LICENSE_WITH_YEAR = "license/TestLicencseWithYear"; |
There was a problem hiding this comment.
s/TestLicencseWithYear/LicenseWithYear/ ?
| .testUnaffected(getFileContentWithYEAR(currentYear())) | ||
| .testUnaffected(getFileContentWithYEAR("2003")) | ||
| .testUnaffected(getFileContentWithYEAR("1990-2015")) | ||
| .test(getFileContentWithYEAR("not a year"), getFileContentWithYEAR(currentYear())); |
There was a problem hiding this comment.
Unless I've misunderstood something, I think that this line should be like the following code snippet, so that we test that Spotless replaces $YEAR with the current year:
.test(getFileContentWithYEAR("$YEAR"), getFileContentWithYEAR(currentYear()));If LicenseHeaderStep.java currently replaces text like "not a year" with the current year, then it should absolutely be fixed to recognise "$YEAR" and "$YEAR" only.
There was a problem hiding this comment.
We want to check here that the regex [0-9]{4}(-[0-9]{4})? detect that the content in the current file is not in a good format.
not a year is not in a good format, so it should be replaced by a license having the current year.
|
I'm was a little concerned at first by how this new feature would introduce volatility to license header checks through the dependence on There are a few places here and there where I think the formatting and grammar could be improved, but I seem to remember that the last time we discussed formatting, @nedtwigg, you expressed the opinion that as long as the formatting passes Spotless's checks then it's all fine. So I'm happy to address any niggles in my own PR if I have the time. :) I've also noticed some areas for improvement which I think don't quite count as niggles and should be addressed or at least discussed further. I've raised these in a recent review. tl;dr: Once my review comments are addressed and/or discussed, I'd be happy to give a "LGTM"! |
|
I did some naming change to make the test easier to understand. Don't hesitate to tel me if it needs more change. |
|
Many thanks for the changes you made so far @baptistemesta! I've had an initial look over your responses, but things have been busier than usual for me recently due to Christmas and other IRL things, so I'll aim to get back to you within the next 2 days. :) |
| * You should have received a copy of the GNU Lesser General Public License along with this | ||
| * program; if not, write to the Free Software Foundation, Inc., 51 Franklin Street, Fifth | ||
| * Floor, Boston, MA 02110-1301, USA. | ||
| **/ |
There was a problem hiding this comment.
Just the concept of having a GNU licence anywhere in the source code is kinda a red flag.
Since the GNU license itself is licensed under GNU and we are "linking" (can be interpreted broadly) against the licence by reading in the file perhaps it would be better to pick a different licence with a year token.
There was a problem hiding this comment.
While I totally respect GNU I don't want it anywhere near my software. 😆
There was a problem hiding this comment.
haha, I will change that!
| licenseHeaderBeforeYEARToken = licenseHeader.substring(0, yearTokenIndex); | ||
| licenseHeaderAfterYEARToken = licenseHeader.substring(yearTokenIndex + 5, licenseHeader.length()); | ||
| licenseHeaderWithYEARTokenReplaced = licenseHeader.replace("$YEAR", String.valueOf(YearMonth.now().getYear())); | ||
| this.yearMatcherPattern = Pattern.compile("[0-9]{4}(-[0-9]{4})?"); |
There was a problem hiding this comment.
Minor niggle: I believe it would read a bit better if all field usages in this constructor were prepended with this..
For example, hasYearToken = ... refers to a field, but the hasYearToken part lacks a this., which makes it a bit inconsistent with the rest of the code in the constructor.
| if (matcher.start() == licenseHeader.length() && raw.startsWith(licenseHeader)) { | ||
| if (hasYearToken) { | ||
| if (matchesLicenseWithYearToken(raw, matcher)) { | ||
| //that means we have the license like `licenseHeaderBeforeYEARToken 1990-2015 licenseHeaderAfterYEARToken` |
There was a problem hiding this comment.
Minor niggle: There should ideally be a space in between // and that to be consistent with the other comments.
| int yearTokenIndex = licenseHeader.indexOf("$YEAR"); | ||
| licenseHeaderBeforeYEARToken = licenseHeader.substring(0, yearTokenIndex); | ||
| licenseHeaderAfterYEARToken = licenseHeader.substring(yearTokenIndex + 5, licenseHeader.length()); | ||
| licenseHeaderWithYEARTokenReplaced = licenseHeader.replace("$YEAR", String.valueOf(YearMonth.now().getYear())); |
There was a problem hiding this comment.
Minor niggle: I'd ideally like to see instances of YEAR in field names turned into Year, to be consistent with the rest of the code.
Just to clarify, there's no need to change strings like $YEAR starting with a dollar sign to $Year - just field names. :)
| private boolean matchesLicenseWithYearToken(String raw, Matcher matcher) { | ||
| int startOfTheSecondPart = raw.indexOf(licenseHeaderAfterYEARToken); | ||
| return (raw.startsWith(licenseHeaderBeforeYEARToken) && startOfTheSecondPart + licenseHeaderAfterYEARToken.length() == matcher.start()) | ||
| && yearMatcherPattern.matcher(raw.substring(licenseHeaderBeforeYEARToken.length(), startOfTheSecondPart)).matches(); |
There was a problem hiding this comment.
Again to clarify, usages of this. are not needed here, as I believe the unwritten convention in Spotless is that this. is only required when referring to fields in constructors.
plugin-gradle/README.md
Outdated
|
|
||
| ## License header options | ||
|
|
||
| The license header can contains a `$YEAR` variable that will be replaced by the current year. |
There was a problem hiding this comment.
I wonder if this sentence would read better as something like the following:
If the string contents of a
licenseHeaderstep or the file contents of alicenseHeaderFilestep contains a$YEARtoken, then in the end-result generated license headers which use this license header as a template,$YEARwill be replaced with the current year.
plugin-gradle/README.md
Outdated
| ``` | ||
| /* Licensed under Apache-2.0 2017. */ | ||
| ``` | ||
| if build is launched in 2017 |
There was a problem hiding this comment.
I wonder if this would read better as:
if Spotless is launched in 2017.
plugin-gradle/README.md
Outdated
| * The license is not formatted correctly | ||
| * It will *not* replace the license when | ||
| * The year variable is already present and is a single year, e.g. `/* Licensed under Apache-2.0 1990. */` | ||
| * The year variable is already present and is a year span, e.g. `/* Licensed under Apache-2.0 1990-2003. */` |
There was a problem hiding this comment.
I wonder if these rules would read better as:
The `licenseHeader` and `licenseHeaderFile` steps will generate license headers with automatic years from the base license header according to the following rules:
* A generated license header will be updated with the current year when
* the generated license header is missing
* the generated license header is not formatted correctly
* A generated license header will _not_ be updated when
* a single year is already present, e.g.
`/* Licensed under Apache-2.0 1990. */`
* a hyphen-separated year range is already present, e.g.
`/* Licensed under Apache-2.0 1990-2003. */`
* the `$YEAR` token is otherwise missing
@baptistemesta Is my understanding correct that no year substitution will happen if $YEAR is not present in the base license header?
|
Hey @baptistemesta, have you had the time recently to have a look at my latest review? Is there anything I can do to help move this PR forward? :) |
|
Hi. |
|
Since the last PR was just style niggles, and this is a very timely feature, I'm going to release right now. Feel free to continue to improve docs or style in a new PR if you would like :) |
|
Published in 3.8.0. |
|
If you care about license headers, you should checkout our new |
This allow to have a
yearvariable in the license file and to avoid having the code reformatted every year.This also avoid having to change the year from the license file every year...
This token is replaced by the current year in the following cases: