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

incomplete parsing when weekday is defined twice #28

Open
nlehuby opened this issue Dec 6, 2018 · 4 comments
Open

incomplete parsing when weekday is defined twice #28

nlehuby opened this issue Dec 6, 2018 · 4 comments

Comments

@nlehuby
Copy link

nlehuby commented Dec 6, 2018

The opening_hours of this place are: Mo-Fr 11:30-15:00, We-Mo 18:00-23:00

The plain text description goes like this:

>>> oh = hoh.OHParser(field, locale="en")
>>> print(oh.plaintext_week_description(year=2018, weeknumber=1, first_weekday=0))
Monday: 6:00 PM – 11:00 PM
Tuesday: 11:30 AM – 3:00 PM
Wednesday: 6:00 PM – 11:00 PM
Thursday: 6:00 PM – 11:00 PM
Friday: 6:00 PM – 11:00 PM
Saturday: 6:00 PM – 11:00 PM
Sunday: 6:00 PM – 11:00 PM

whereas the expected is:

Monday: 11:30 AM – 3:00 PM and 6:00 PM – 11:00 PM
Tuesday: 11:30 AM – 3:00 PM
Wednesday: 11:30 AM – 3:00 PM and 6:00 PM – 11:00 PM
Thursday: 11:30 AM – 3:00 PM and 6:00 PM – 11:00 PM
Friday: 11:30 AM – 3:00 PM and 6:00 PM – 11:00 PM
Saturday: 6:00 PM – 11:00 PM
Sunday: 6:00 PM – 11:00 PM
@francois2metz
Copy link

I created a failing test case here: francois2metz@9f726b1

@rezemika rezemika added the bug label Dec 10, 2018
@rezemika
Copy link
Owner

Thank you for your report!

It seems quite complicated to fix... I'll take a look in the code ASAP, but I'm not sure it's currently possible to fix this without rewriting a consequent part of the parsing logic (something that I'd like to do, but I currently don't have time).

@francois2metz
Copy link

I looked at the code and managed to fix the problem, while breaking all other features ;)

I don't know if it's feasible to merge some rules when they have the same priority in the function get_current_rule. What do you think?

@rezemika
Copy link
Owner

Of course, it would be great, but it must be quite difficult to check compatibility of rules before merging. Theoretically, only rules separated by commas should be merged, and rules separated by semicolons should be mutually exclusive. It may be possible to make a patch, but I think a clear handling of that would require an important rewrite of the parsing logic. The grammar specifications are so complicated...

Currently, the main reason the commas are supported is that they're massively wrongly used. They should be used only to merge opening hours of many days spanning over midnight, and many field use them as semicolons. Actually, the parser considers commas as synonyms of semicolons for this reason, because opening hours spanning over midnight are already supported with semicolons (because the parser does not care of the separator).

Also, I think this problem is related with another: when a specific period in a day is defined as closed, the whole day is considered as closed. For example, with Mo-Sa 08:00-19:00; Fr 12:00-14:00 off, the whole friday is considered as closed (to prevent problems, I raise a ParseError in this case).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants