Split instant() to two simpler functions instant() and instantFromText()#69
Merged
Merged
Conversation
Contributor
Author
|
Note that while looking for call sites and docs to update, I found some under the |
Current signature of zeit.instant() is:
fn instant(io: std.Io, config: Instant.Config) !Instant;
which forces unnecessary complexity at call site, because:
* it's fallible (`!Instant`), but the fallibility is only applicable
when parsing text, ie. when config.source is initialized with
certain tags,
* and it always accepts std.Io but this is only relevant when
config.source is initialized with `.now` tag.
This commit splits current .instant() into two simpler functions:
fn instant(source: Instant.Source, tz: *const TimeZone) Instant;
where accepted union contains `std.Io` only when necessary, ie. as
payload for the `.now` tag, and
fn instantFromText(format: Instant.TextFormat, text: []const u8, tz: *const TimeZone) !Instant;
where *TextFormat* enum contains only recognized formats (eg.
`.iso8601`).
Notice that in the first case, even though .instant() can talk to
std.Io, the call is to system clock, which is infallible.
Also I've "smuggled in" further simplification: timezone information is
passed as separate argument, instead of `Instant.Config`, leading to
more readable call sites in both cases, as demonstrated in README.md
and tests:
const now = try zeit.instant(io, .{});
⬇️
const now = zeit.instant(.{.now = io}, &zeit.utc);
..
const epoch = try zeit.instant(std.testing.io, .{ .source = .{ .unix_timestamp = 0 } });
⬇️
const epoch = zeit.instant(.{ .unix_timestamp = 0 }, &zeit.utc);
and
_ = try zeit.instant(std.testing.io, .{
.source = .{
.rfc3339 = "2024-03-16T08:38:29.496706064-1200",
},
});
⬇️
_ = try zeit.instantFromText(
.rfc3339,
"2024-03-16T08:38:29.496706064-1200",
&zeit.utc,
);
The readability comes at a small price that timezone, which would
default to UTC in the older API, now *must* be passed, but this is
trivial, easy to learn from the README.md and arguably better as in
"more explicit".
5398324 to
1bbc246
Compare
Contributor
Author
|
(force-pushing to remove period prefixes from the commit message---added by me out of weird habit.) |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Current signature of zeit.instant() is:
which forces unnecessary complexity at call site, because:
it's fallible (
!Instant), but the fallibility is only applicable when parsing text, ie. when config.source is initialized with certain tags,and it always accepts std.Io but this is only relevant when config.source is initialized with
.nowtag.This commit splits current .instant() into two simpler functions:
where accepted union contains
std.Ioonly when necessary, ie. as payload for the.nowtag, andwhere TextFormat enum contains only recognized formats (eg.
.iso8601).Notice that in the first case, even though .instant() can talk to std.Io, the call is to system clock, which is infallible.
Also I've "smuggled in" further simplification: timezone information is passed as separate argument, instead of
Instant.Config, leading to more readable call sites in both cases, as demonstrated in README.md and tests:⬇️
..
⬇️
and
⬇️
The readability comes at a small price that timezone, which would default to UTC in the older API, now must be passed, but this is trivial, easy to learn from the README.md and arguably better as in "more explicit".