Skip to content
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

fix day range parsing (zk-org/zk#382) #384

Merged
merged 1 commit into from
Jan 22, 2024
Merged

fix day range parsing (zk-org/zk#382) #384

merged 1 commit into from
Jan 22, 2024

Conversation

tjex
Copy link
Member

@tjex tjex commented Jan 22, 2024

fix: set day range end to be one second before midnight.

i.e, the next day begins at 00:00.

Now the day range extends to 23:59:59. Meaning that the notes created within the
same day, that do not have time stamps, will be returned by a query such as zk list --created=today, previously would fall under ... --created=yesterday.

Likewise, given a note with the YAML date 2024-01-22, zk would report this as being
created on 2024-01-21.

fix: set day range end to be one second before midnight.

i.e, the next day begins at 00:00.
Copy link
Member

@jurica jurica left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds sensible and change LGTM.

@jurica jurica merged commit 537c7f7 into zk-org:main Jan 22, 2024
3 checks passed
@tjex tjex deleted the day-parsing branch January 23, 2024 08:41
@mcDevnagh
Copy link
Contributor

Could we get a test for this fix, so we don't see a regression?

Like the following:

func TestFormatDateHelperElapsedYear(t *testing.T) {
but in this file: https://github.com/zk-org/zk/blob/main/internal/cli/filtering_test.go

It would need to fail before your changes, and succeed with the'

@tjex
Copy link
Member Author

tjex commented Jan 24, 2024

yesss. good idea 😅
Will do so.

Althought, it should actually be tested with the tesh suite or?

The go tests in filtering_test.go aren't operating on any actual example data from what I see.
They're just testing API breakages?.. If that's the right way to term it.

In other words, 'today' will always pass if the function executes and returns successfully.

So by that token, the current tesh tests should already be covering this change as in tests/cmd-list-filter-date.tesh there is already a test for --created today, and it was still passing after this PR.

But, perhaps there aren't any test notes that have a time stamp of 00:00:00, which was the edge case, so will investigate that anyway in the meantime.

start = startOfDay(day)
// we add -1 second so that the day range ends at 23:59:59
// i.e, the 'new day' begins at 00:00:00
start = startOfDay(day).Add(time.Second * -1)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What was the value of start before? 00:00:00 or 00:00:01 ?
If start was already 00:00:00 then it seems that subtracting a second should be done to from end.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Logically that does make sense in how I would also think a day range should be. But the issue is that when a note is created without a time stamp in YAML, that it's given the time 00:00. But it would seem the time library considers 00:00 to be midnight of the upcoming change over to the next day.

So if the second was subtracted from the end, and you made a note without a time stamp today, it would appear in a query for notes being created tomorrow (even though I don't think that query exists? It's just to illustrate the point)

In any case, subtracing 1 second from the end of the day, causes the issue-382.tesh test to fail. That test is a note created date: 2024-01-24 (without a time stamp), and queried with zk list -q --created 2024-01-24.

Is there another way to work around this that keeps a more logical way of interpreting a day range?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here's the test: #385

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants