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

relative day and month expressions #31

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Conversation

xuezhma
Copy link

@xuezhma xuezhma commented Mar 22, 2016

as initially discussed in issue #30

@@ -135,11 +135,11 @@ DurationString
DurationAbbrev
= "ms"
/ "s"
/ "m"
/ "m" !"o"
Copy link
Collaborator

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

@xuezhma
Copy link
Author

xuezhma commented Mar 22, 2016

@welch sounds good. Will try to make these changes.

@xuezhma
Copy link
Author

xuezhma commented Mar 22, 2016

@welch
Let's say we add weekday and month to CalendarUnit.

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:

// January for example
{
  calendarUnit: 'year',
  momentUnit: 'month', 
  value: 0
} 

// "next" for example 
/ "next" _ unit:Unit {
        return {
            type: "CalendarExpression",
            valueType: "moment",
            direction: "down",
            unit: unit.calendarUnit,
            expression: {
                type: "BinaryExpression",
                operator: "+",
                left: {
                    type: "NowLiteral"
                },
                right: {
                   type: "MomentDuration",
                   value: unit.value,
                   unit: unit.momentUnit
               }
            }
        };
    }

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?

@welch
Copy link
Collaborator

welch commented Mar 22, 2016

those are interesting proposals, thanks for working through them.
I will get back to you tomorrow after I've had a chance to think some more about this.

@xuezhma
Copy link
Author

xuezhma commented Mar 30, 2016

Any thoughts?

@welch
Copy link
Collaborator

welch commented Mar 30, 2016

Apologies for the delay! Been a little swamped here.
I will try to get this turned around for you in the next day or so.

@hansent
Copy link

hansent commented Jun 14, 2016

Any updates on this?

Thanks for the awesome library!

@welch
Copy link
Collaborator

welch commented Jun 16, 2016

I've not forgotten this, and thank you for the nudge.
I will make some time to pick this back up this week.
Glad you find it of use!

@welch
Copy link
Collaborator

welch commented Jun 20, 2016

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).

@xuezhma
Copy link
Author

xuezhma commented Jun 20, 2016

I'm on it

@xuezhma
Copy link
Author

xuezhma commented Jun 21, 2016

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.

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.

None yet

3 participants