-
Notifications
You must be signed in to change notification settings - Fork 273
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
feat(template): add date template helper functions #5997
Conversation
Thanks so much for the PR @ManAnRuck! ❤️ I think it definitely makes sense to have a helper function for printing the current timestamp with options for different formats. Adding functionality for manipulating dates feels a little out of scope though tbh and I can see this grow in until we've re-implemented all of date-fs as template functions. What exactly is your use case? Wondering if there's other ways to achieve that. |
Hi @eysi09 Thank you for your response :) That would of course be ideal if we could find a solution without having to introduce additional complexity into the code :) Specifically, the use case is that an environment variable exists for scraper services and api services, which defines the date from which the scraper or the data should be retrieved. It is of course possible to enable this in the code itself using another environment variable, but this would be logic that is not part of the business logic of the actual service and I would therefore like to avoid incorporating it into the service. For example, at the time the service/job is started, all data that is newer than 7 days for dev should be pulled. for production, the environment variable is set based on the date of the last update. |
Another alternative could be to make it possible to add your own scripts, which would be very good in the sense that it could work like a plugin system, but I imagine the implementation to be a bit more complex. For the user it could then look like this: there is a standard file path or/and this can be defined in garden.yaml files. The biggest hurdles from my point of view would be the use of third party libraries for own functions (e.g. |
I discussed this a bit better with the team. We agree that adding all these helpers functions is a lot and it kind of pollutes the function namespace because we don't have the functionality to tie to some context (e.g. Also note that we already have the Would it work for your use case if instead we just:
That modifyDate(timestamp: string, unit: "ms" | "s" | "d" | "m" | "y", value: number, format?: string) The It could be called like this in the YAML config: yesterday: ${modifyDate(datetime.now, "d", -1)} The |
Thank you for the discussion and suggestions regarding the date handling functions. ❤️ I've taken the liberty to flesh out the modifyDate function as per the discussions, ensuring it accommodates both "add" and "set" modes. This allows for greater flexibility, whether adjusting or explicitly setting date units. To avoid ambiguity, especially in units that could be easily conflated like months ('m') and minutes ('minutes'), I opted to use fully spelled-out units. This should help prevent any potential confusion or errors in unit interpretation. Additionally, I added a
yesterday: ${modifyDate(datetime.now, "days", -1)}
first-day: ${modifyDate(datetime.now, "days", 1, "set")} I would be happy if you could rate these adjustments again and share your thoughts :) (the code is already customized) |
58c4c8b
to
1d52f3f
Compare
045cbf5
to
cece34f
Compare
{ input: ["2021-01-01T00:00:00.234Z", 345, "milliseconds"], output: "2021-01-01T00:00:00.345Z" }, | ||
{ input: ["2021-01-01T00:00:05Z", 30, "seconds"], output: "2021-01-01T00:00:30.000Z" }, | ||
{ input: ["2021-01-01T00:01:00Z", 15, "minutes"], output: "2021-01-01T00:15:00.000Z" }, | ||
{ input: ["2021-01-01T12:00:00Z", 11, "hours"], output: "2021-01-01T11:00:00.000Z", skipTest: true }, |
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.
These tests likely fail becaue we instantiate a date with local timezone from the UTC string, and then we modify the local time zone and export as UTC again.
In the first iteration of this, my proposal would be to rename shiftDate
, modifyDate
and formatDate
to shiftDateUTC
, modifyDateUTC
and formatDateUTC
and to only operate UTC dates; if someone needs timezone support later we can consider that by adding formatDateLocal
or similar functions.
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.
Do we not also need nowUTC()
or something like that?
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.
These tests likely fail becaue we instantiate a date with local timezone from the UTC string, and then we modify the local time zone and export as UTC again.
In the first iteration of this, my proposal would be to rename
shiftDate
,modifyDate
andformatDate
toshiftDateUTC
,modifyDateUTC
andformatDateUTC
and to only operate UTC dates; if someone needs timezone support later we can consider that by addingformatDateLocal
or similar functions.
Did that for date modifiers in 890adc0. The formatDate
will become formatDateUtc
in the next commit :)
Do we not also need
nowUTC()
or something like that?
We can do that, it might be useful.
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.
And the formatter was updated in 96b2300.
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.
I'm not sure if we need nowUtc()
now. @ManAnRuck wdyt about it?
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.
I think if it helps we can leave it out for now and from the user's point of view a workaround with a manual adjustment according to the time zone may have to be made first. Then we can first collect what problems may arise and improve this in a further integration.
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.
Ok, thank you! Let's collect some feedback and do the necessary improvement in the further PR(s).
We're merging this PR today :)
Signed-off-by: Manuel Ruck <[email protected]>
The hierarchical switch statements are hard to maintain. The `mode` argument served as a routing param to pick up necessary function behaviour. It's better to get rid of it and to maintain 2 independent functions. Each function can have its own limitations and time unit types.
TODO: * consider binding `ShiftDateTimeUnit` to `Duration` from `date-fns` * add more tests
Milliseconds are not supported by the underlying `add` function.
Type `Function` is not recommended to use by `@typescript-eslint/ban-types`. Let's use a bit more specific function interface.
98b6a57
to
0876da8
Compare
@stefreak @ManAnRuck please review the current version of the PR. Comments, feedback and improvement ideas are highly appreciated :) cc @eysi09 |
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.
Thanks for the contribution @ManAnRuck, I think this can be helpful for some folks 👍
Thanks for revisiting @vvagaytsev
What this PR does / why we need it:
See Discord Thread for the details.
Which issue(s) this PR fixes:
Fixes #5995
Special notes for your reviewer:
do someone maybe have an idea to ensure the dates are always in the same timezone for the tests?