feat: allow date updating in generic strategy #2440
Conversation
There was a problem hiding this comment.
Thank you for this, and apologies for the delayed review.
This all seems reasonable (with a few minor nits), except that the special version-date replacement seems a bit arbitrary. In the example test, the java code could have separate constants for the version and the date and format it separately in code.
Is there a use case where this is generally needed?
src/updaters/generic.ts
Outdated
|
|
||
| constructor(options: GenericUpdateOptions) { | ||
| super(options); | ||
|
|
||
| this.inlineUpdateRegex = options.inlineUpdateRegex ?? INLINE_UPDATE_REGEX; | ||
| this.blockStartRegex = options.blockStartRegex ?? BLOCK_START_REGEX; | ||
| this.blockEndRegex = options.blockEndRegex ?? BLOCK_END_REGEX; | ||
| this.date = options.date ?? new Date(); | ||
| this.dateFormat = options.dateFormat ?? '%Y-%m-%d'; |
There was a problem hiding this comment.
nit: can we define this as a constant?
src/strategies/base.ts
Outdated
| @@ -148,6 +150,7 @@ export abstract class BaseStrategy implements Strategy { | |||
| this.extraFiles = options.extraFiles || []; | |||
| this.initialVersion = options.initialVersion; | |||
| this.extraLabels = options.extraLabels || []; | |||
| this.dateFormat = options.dateFormat || '%Y-%m-%d'; | |||
There was a problem hiding this comment.
nit: define as a constant like DEFAULT_DATE_FORMAT in this file
There was a problem hiding this comment.
I set this as a constant in src/updaters/generic.ts and imported it from there.
Hm, I'm not sure I understand what you mean by arbitrary here?
Do you mean the functionality in general? If so, then my use case is latex packages, that have this: \ProvidesPackage{beamercolorthememoloch-highcontrast}[2025-01-17 v0.5.0 Moloch color theme]But there is also #1798, where @aaronware needed it for another purpose. Thanks for the review! I'll fix the issues momentarily. |
Allow updating of dates using the generic updater, by adding a x-release-please-date or x-release-please-version-date (to update both version and date) to a file anywhere. The implementation is still very basic. The date is retrieved by just checking the current date, but a better approach would be to look at the timestamp of the previous feat/fix or breaking change for a conventional commit, I think. I've added an option --date-format to specify the date format. There are no assertions with respect to this right now, and the regex matching can be improved in several ways. One option I considered was to try to auto-detect the date format, but I think this is bound to be problematic because there are ambiguities in date formatting. Fixes googleapis#1798
src/bin/release-please.ts
Outdated
| @@ -346,6 +346,10 @@ function pullRequestStrategyOptions(yargs: yargs.Argv): yargs.Argv { | |||
| describe: 'Override the detected latest tag name', | |||
| type: 'string', | |||
| }) | |||
| .option('date-format', { | |||
| describe: 'format in strftime format for updating dates', | |||
| default: 'string', | |||
There was a problem hiding this comment.
Sorry, I missed this on the first review. This was probably meant to be type and/or the default value should be %Y-%m-%d (or left as nullable and rely on the default value set in code).
|
Ah, of course, yes, thank you for catching that. I'll set it to `type: 'string'`. Not sure what's best in terms of setting the default or not. I'll leave it as nullable for now.
|
Allow updating of dates using the generic updater, by adding a x-release-please-date or x-release-please-version-date (to update both version and date) to a file anywhere.
The implementation is still very basic. The date is retrieved by just checking the current date, but a better approach would be to look at the timestamp of the previous feat/fix or breaking change for a conventional commit, I think.
I've added an option --date-format to specify the date format. There are no assertions with respect to this right now, and the regex matching can be improved in several ways.
One option I considered was to try to auto-detect the date format, but I think this is bound to be problematic because there are ambiguities in date formatting.
Fixes #1798 🦕