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

Incorrect parsing of bare day of week, e.g. "friday" #137

Open
fredcallaway opened this issue May 24, 2023 · 11 comments · May be fixed by #153
Open

Incorrect parsing of bare day of week, e.g. "friday" #137

fredcallaway opened this issue May 24, 2023 · 11 comments · May be fixed by #153
Labels
help wanted Accepting PRs for this feature requests

Comments

@fredcallaway
Copy link

I think this is related #81, which was closed. Possible regression?

I have Monday as my locale date, but the behavior is the same if I change it to Sunday in NLD settings.

I generated these using shift (right of pipe is what I typed, left is output)

[[2023-05-24|Today]]
[[2023-05-24|wednesday]]
[[2023-05-25|this Thursday]]
[[2023-05-25|thursday]] 👌
[[2023-05-26|this Friday]] 
[[2023-05-19|friday]] ❌
@josha1971
Copy link

I ran into this issue also today.

Typing @saturday, @sunday, @thursday or @friday will produce the previous week's date for those days ... oddly typing @tuesday or @wednesday will produce this week's day (it's Monday the 19th today when I'm doing these tests)

@jose-elias-alvarez
Copy link

jose-elias-alvarez commented Sep 4, 2023

I am not a date parsing expert, but quick testing indicates that Chrono's forwardDate option (which was suggested in #81) solves this particular issue.

I don't fully understand the nature of the fix in #83 but it seems that adding the option here solves both this issue and #81, without the need to forward locale and fork the upstream library (unless that was intended to solve a separate issue, though it doesn't seem necessary for #64). I'm happy to put in a PR if this seems like the right fix.

@pierremouchan
Copy link

Hey @jose-elias-alvarez any update on this, did you solve the issue on your end? thanks a lot

@garbosz
Copy link

garbosz commented Feb 3, 2024

im seeing a similar issue to this where @next wednesday (should show 2/7 in this case) will always return the same date as @wednesday (returning 1/31) (date currently 2/3)

@argenos argenos added the help wanted Accepting PRs for this feature requests label Feb 16, 2024
@matthewspear
Copy link

matthewspear commented Feb 27, 2024

I'd love this feature too and got super confused why I had dates from the week before in all my Todos!

Happy to contribute – What is the desired behaviour?

Is it as simple as parsing any direct dates e.g @monday, @sunday as the equivalent of this @monday or this @sunday

If say the day of the week is Wednesday and you use @monday do you expect the past Monday or future coming Monday to be added?

Would week start effect any of this?

In my laziness I just added this into a local copy in parser.ts somewhere around line 85:

const dayDateMatch = selectedText.match(/^(monday|tuesday|wednesday|thursday|friday|saturday|sunday)$/i);

if (dayDateMatch) {
  return parser.parseDate(`this ${dayDateMatch[0]}`, referenceDate, { 
    locale,
  });
}

Of course it'd be nice to have shortened versions too like @mon but I just needed a quick fix for myself for now!

Edit: fixed regex :)

@garbosz
Copy link

garbosz commented Feb 27, 2024

I'd love this feature too and got super confused why I had dates from the week before in all my Todos!

Happy to contribute – What is the desired behaviour?

Is it as simple as parsing any direct dates e.g @monday, @sunday as the equivalent of this @monday or this @sunday

If say the day of the week is Wednesday and you use @monday do you expect the past Monday or future coming Monday to be added?

Would week start effect any of this?

In my laziness I just added this into a local copy in parser.ts somewhere around line 85:

const dayDateMatch = selectedText.match(/^(monday|tuesday|wednesday|thursday|friday|saturday|sunday)$/i);

if (dayDateMatch) {
  return parser.parseDate(`this ${dayDateMatch[0]}`, referenceDate, { 
    locale,
  });
}

Of course it'd be nice to have shortened versions too like @mon but I just needed a quick fix for myself for now!

Edit: fixed regex :)

i think what would be intuitive is you could look at how people tend to refer to days of the week and have the @monday with no modifiers just grab the nearest monday(for your wednesday example it would grab the past monday since its closer then the future monday) and then have a @lastMonday or @nextmonday modifier to force past or future.

@fredcallaway
Copy link
Author

The plugin is running up against the imprecision of natural language. What people mean when they say "monday" is highly contextual—in particular it depends a lot on the tense of nearby verbs (compare "are you gonna watch the game on monday?" to "did you see the game monday?") Similarly, when I say "next monday" on Friday, I could be referring to either 3 or 10 days from now. On monday, "next friday" could be 4 or 11 days from now.

Given this, I think the plugin should not try so hard to resemble natural speech in this case and instead just implement something simple and easy to predict. I think the current algorithm is close to @garbosz's proposal (finding the nearest monday). This issue suggests that this is confusing to people.

I suggest that "this monday" and "monday" be equivalent and they both refer to the monday in the current week (defined by the locale-specific week start). "Next monday" should always refer to one week after "monday" and "last monday" should always refer to one week before "monday". Is this perfect? No. But it's simple and I think most people would quickly understand it after experimenting a bit.

A natural user option would be to make bare "monday" and/or "this monday" always refer to an upcoming date (with next/last offset from there). And I suppose you could add "nearest" while you're at it. But in another issue I think @argenos said that they don't want to add configurable options for this.

@argenos
Copy link
Owner

argenos commented Feb 28, 2024

I suggest that "this monday" and "monday" be equivalent and they both refer to the monday in the current week (defined by the locale-specific week start).

FWIW, this should be the current behaviour (at least that's what I attempted way back when I developed this). I know other people interpret "Monday" and "this Monday" differently, but a) I don't currently have time to add this feature and b) a PR that adds configuration options to interpret this, will only add edge cases and technical debt in the future, making the maintenance even more difficult. I am open to merge a solution, if it's simple (even if it has a configuration for this).

To whomever is interested in contributing a solution to this: please explain below or in a separate issue how you'd go about it, and then once we have agreed, please open (several) PRs! Make PRs in small chunks please, so I can review and approve, it's unlikely I'll have time to sit and review large/significant changes to the code-base.

@jack2game
Copy link

jack2game commented Mar 12, 2024

Linking similar issue here #111

Cause of Issue

I believe the strange behavior is from chrono: https://github.com/wanasit/chrono/blob/57676b0049e3d2712418f0bb47b047f8fb3fa670/src/common/calculation/weekdays.ts#L10

modifier "this", "next", "last" modifier word. If empty, returns the weekday closest to the refDate

Furthermore, the author added extra confusion by manipulating "Next" on Sat and Sun, where:

            // From Sunday, the next Sunday is 7 days later.
            // Otherwise, next Mon is 1 days later, next Tues is 2 days later, and so on..., (return enum value)
            // From Saturday, the next Saturday is 7 days later, the next Sunday is 8-days later.
            // Otherwise, next Mon is (1 + 1) days later, next Tues is (1 + 2) days later, and so on...,
            // (return, 2 + [enum value] days)
            // From weekdays, next Mon is the following week's Mon, next Tues the following week's Tues, and so on...
            // If the week's weekday already passed (weekday < refWeekday), we simply count forward to next week
            // (similar to 'this'). Otherwise, count forward to this week, then add another 7 days.

This, to me, is inconsistent and actually reduces the usability of the function.

nldates-obsidian is currently using a fork of chrono based on modifications from @liamcain, but still suffers inconsistency, with the usage of .weekday(), although not as bad.

I think the issue can be solved in several steps. The key for each one is removing ambiguity. I propose following improvement.

Step 1: Bare day of week = "This"

Input Result
Monday This Monday
Tuesday This Tuesday
Wednesday This Wednesday
Thursday This Thursday
Friday This Friday
Saturday This Saturday
Sunday This Sunday

This is the easy part.

Step 2: Slice weeks according to settings.weekStart

  • Depending on where Today is, and the Week Starts on settings, slice the dates to "Last (yellow)", "This (green)", "Next (blue)" accordingly
  • This should always be the week that Today currently sits in, then you'll have Last and Next
  • This is similar to This Week, This Month, This Year, when we say This, we always mean the current period of time that Today sits in
  • Some more examples can be found in the pic below, for easier understanding

312052721-d3c4dc2f-6989-4c4c-b806-3e81e0cf4b9f

I'm pretty new to developing plugin for Obsidian, therefore not sure if I have the capability to implement this improvement, but imho this is pretty much the optimal way of removing ambiguity.

Finally, if I implement this, PR to this particular repo will be very minimum (only line actually), given most of improvements needs to happen in chrono, not nldates

@jack2game
Copy link

jack2game commented Mar 12, 2024

The actual fix was way easier than I expected. I have a working version now. Please let me know your thoughts @argenos . Thanks

@argenos
Copy link
Owner

argenos commented Apr 11, 2024

@jack2game Thanks! Sounds great so far! Would you mind opening a PR? I can then review what this implies exactly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Accepting PRs for this feature requests
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants