-
Notifications
You must be signed in to change notification settings - Fork 46
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
Comments
I am not a date parsing expert, but quick testing indicates that Chrono's 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 |
Hey @jose-elias-alvarez any update on this, did you solve the issue on your end? thanks a lot |
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) |
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 If say the day of the week is Wednesday and you use Would week start effect any of this? In my laziness I just added this into a local copy in 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 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. |
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. |
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. |
Linking similar issue here #111 Cause of IssueI believe the strange behavior is from
Furthermore, the author added extra confusion by manipulating "Next" on Sat and Sun, where:
This, to me, is inconsistent and actually reduces the usability of the function.
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"
This is the easy part. Step 2: Slice weeks according to
|
The actual fix was way easier than I expected. I have a working version now. Please let me know your thoughts @argenos . Thanks |
@jack2game Thanks! Sounds great so far! Would you mind opening a PR? I can then review what this implies exactly. |
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.
The text was updated successfully, but these errors were encountered: