-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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 a single weekday grammar callback #4357
Conversation
e5e46c4
to
59ecf0a
Compare
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.
The part around the single central line of code looks solid and makes me believe that the fix does what it is supposed to.
However, I feel that we could do a better job of documenting what spirit syntax does. I can't even begin to tell what the function does and why these two braces change it from false to correct.
= wday[_a = _1, _b = _1] | ||
>> -(('-' >> wday[_b = _1]) | ||
| ('[' >> (nth_entry % ',') >> ']' >> -day_offset)) | ||
= (wday[_a = _1, _b = _1] |
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.
@oxidase I have a very hard time understanding this magic syntax. Given that these changes are not that obvious, do you think that we could add some commentary to make it easier to follow what is supposed to happen here?
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.
@MoKob i can explain what happened in this case: without parenthesis the semantic action [_val = ph::construct<OpeningHours::WeekdayRange>(_a, _b)]
that constructs a WeekdayRange
was invoked only in a case if both the first and the second part of the rule succeed.
In case Mo-Fr
local rule variable _a
will be Mo
and _b
will be Fr
. So the actions will be called as ph::construct<OpeningHours::WeekdayRange>(1, 5)
.
If parsing of the second rule failed but the first part succeeded then both _a
and _b
will be initialized to let say Sa
, but construction of a ph::construct<OpeningHours::WeekdayRange>(6, 6)
will not be called. So WeekdayRange
will be a default initialized object with an empty range.
Adding parenthesis around both rules makes invocation of ph::construct<OpeningHours::WeekdayRange>(_a, _b)
in both cases:
- if the first part succeeds, like 'Mo' ->
ph::construct<OpeningHours::WeekdayRange>(1,1)
- if the first and the second optional part succeeds, like 'Tu-Fr' ->
ph::construct<OpeningHours::WeekdayRange>(2,5)
Idk if such lengthy comments would make sense, but I can make a step-by-step intro how to add semantic to unimplemented parts like nth_entry
or day_offset
, For example, with nth_entry
it is possible to define Oct Su[1]
(the first Sunday in October) like in the Oktoberfest example.
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.
@oxidase thank you for this explanation. This makes a lot more sense to me now.
I believe simply having a bit more context on what kind of format the grammar parses might already help here (Parsing Mo-Fr into ph::construct<>(1,5)
or Sa
into ph:construct<6,6>
). I agree that the longer explanation here might be a bit much, but it is already accessible now via git-blame, so I guess we are golden here.
59ecf0a
to
f1e0b19
Compare
Issue
The PR adds a constructor callback for a single weekday condition like
Su 00:00-24:00
Tasklist
Requirements / Relations
Link any requirements here. Other pull requests this PR is based on?