-
Notifications
You must be signed in to change notification settings - Fork 4
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
relative day and month expressions #31
base: master
Are you sure you want to change the base?
Conversation
@@ -135,11 +135,11 @@ DurationString | |||
DurationAbbrev | |||
= "ms" | |||
/ "s" | |||
/ "m" | |||
/ "m" !"o" |
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.
this change, and the others added in subsequent commit, can be eliminated by including Weekday in CalendarUnit, so it is matched before DurationAbbrev, eg,
CalendarUnit
= string:CalendarString "s" {
return string;
}
/ CalendarString
/ Weekday
/ CalendarAbbrev
@welch sounds good. Will try to make these changes. |
@welch CalendarUnit returns a unit, which will be used in both CalendarExpression and MomentDuration while the value is set to 1 as default. But for relative day and month, we kinda more want to set the unit to default (week for weekday's CalendarExpression, day for weekday's MomentDuration, year for month's CalendarExpression, month for month's MomentDuration), but give them different values. I'm thinking maybe we can return an object for CalendarUnit, like:
In this case, we don't need to add any new literal. Or we could just return the weekday/month's name, and check it at HumanDuration and CalendarOffset to determine unit & value there, or simply give them new new literals and figure out the unit & value in the literals. What do you think? |
those are interesting proposals, thanks for working through them. |
Any thoughts? |
Apologies for the delay! Been a little swamped here. |
Any updates on this? Thanks for the awesome library! |
I've not forgotten this, and thank you for the nudge. |
Of the ideas discussed or hinted at, I prefer creating DayNames and MonthNames, recognizing them in most (all?) of the same places as CalendarUnit, and passing them as literals to the generator for handling (with an extra "base" field of 'week' for weekdays and 'year' for month names), so that momentjs can be used to look up proper dayname/monthname offsets instead of wiring that into the grammar. We keep these separate from CalendarUnit so 'base' can be set (should be able to do this within the existing calendar expressions rather than making parallel versions). When the generator evaluates expressions involving daynames, it first evaluates the expression as if the unit were 'week', then backs up to the named day before that week's beginning. Similarly with named months and 'month'. That calendar math seems convoluted, but it gives proper behavior for, eg, 'next wednesday' when today is tuesday, or 'final wednesday of this year' when the final week is partial and doesnt include a wednesday. Let me know if you'd like to take this up, or kick it back to me and I'll try later this week (yeah, I've been a flake on scheduling this work). |
I'm on it |
Rewrote this/next/last/ + weekday/month and just weekday/month. For next weekday/month, I'm giving the weekday/month in next week/year, while for just weekday/month, I'm returning the closest weekday/month from now in the future. To do the latter, I'm having the parser to return the offset number, so it's easier to compare the offset of now with the offset of the input. See changed files for detail. Let me know how this part looks to you. Will add first/final weekday of something next. Don't think the rest places we recognize CalendarUnit will make sense with weekday/month, like first March of this year, Monday 3 of this year. March 3 of this year would happen to sound legit, but the parser will be viewing it as 3 amount of March anyway. Let me know if I'm missing a place to check for either weekday/month. |
as initially discussed in issue #30