feat: allow date updating in generic strategy#2440
feat: allow date updating in generic strategy#2440chingor13 merged 4 commits intogoogleapis:mainfrom
Conversation
chingor13
left a comment
chingor13
left a comment
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
| 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
| 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
chingor13
left a comment
chingor13
left a comment
There was a problem hiding this comment.
Small nit on the CLI options, but otherwise LGTM
src/bin/release-please.ts
Outdated
| }) | ||
| .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.
|
chingor13
force-pushed
the
date-updater
branch
from
bb81a5b to
39c845a
Compare
* feat: allow date updating in generic strategy 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 * fixup! feat: allow date updating in generic strategy * fixup! feat: allow date updating in generic strategy * test: fix snapshot --------- Co-authored-by: Jeff Ching <chingor@google.com>
2 participants
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 🦕