simonpoole / OpeningHoursFragment

Android UI element for displaying and editing an opening hours value
MIT License
9 stars 2 forks source link

easter-Oct Su[2]: Su,PH 11:00-17:00; "außerdem nach Vereinbarung" #18

Closed HolgerJeromin closed 6 years ago

HolgerJeromin commented 6 years ago

easter-Oct Su[2]: Su,PH 11:00-17:00; "außerdem nach Vereinbarung"

Vespucci says this is invalid. validator from @ypid is happy. https://openingh.ypid.de/evaluation_tool/?EXP=easter-Oct%20Su%5B2%5D%3A%20Su%2CPH%2011%3A00-17%3A00%3B%20%22au%C3%9Ferdem%20nach%20Vereinbarung%22&lat=49.9509981&lon=8.0135812&mode=0&DATE=1506894540000

HolgerJeromin commented 6 years ago

http://www.fahrradmuseum-rheinhessen.de/startseite.html https://www.openstreetmap.org/node/526156996

simonpoole commented 6 years ago

As far as I can see from http://wiki.openstreetmap.org/wiki/Key:opening_hours/specification the string seems to be invalid. Potentially Oct 1 +Su+7 days would be a valid variant, the Su[2] syntax would seem to be only valid in a weekday selector not in a date.

HolgerJeromin commented 6 years ago

do you know what is trying to be expressed?

See the website: "An Sonn- und Feiertagen von Ostersonntag bis zum 2. Sonntag im Oktober jeweils von 14-18 Uhr und nach Vereinbarung"

ypid commented 6 years ago

@HolgerJeromin You probably meant easter-Oct Su[2]: Su,PH 14:00-18:00 || "nach Vereinbarung"

Sorry for the confusion. I introduced support for this back in 2013 (ref: https://github.com/opening-hours/opening_hours.js/commit/7e2287184511588a63de6e1b10bba6b2e9f6fa35). This was based on the specification by Netzwolf, (wiki, syntax explained by Netzwolf). I later reworked the specification and created Key:opening_hours/specification based on Netzwolf’s work. Many issues where fixed by this rework, a few issues where added unfortunately. This is one of them (syntax currently not defined/missing). The spec will need to be updated. But before that it should be checked how often this is actually used. The example from @HolgerJeromin shows that there is a need for this.

HolgerJeromin commented 6 years ago

easter-Oct Su[2]: Su,PH 14:00-18:00 || "nach Vereinbarung" is not supported in vespucci either...

simonpoole commented 6 years ago

As alreday said not part of the spec at this point, and imho if included would need a close look if this is is even a sensible thing to support.

simonpoole commented 6 years ago

@ypid while not super frequent there are likely a couple 100 of uses, could you draft a change to the specification?

ypid commented 6 years ago

How should we go about this? Full blown proposal process?

"couple 100 of uses" is a pretty good estimate :-)

$ ./regex_search.js export.opening_hours.json
regex search> \w{3}\s*\w{2}\[\s*-?\s*\d+\s*\]
Matched 268 different values, total in use 385.
Print values? y
Matched (count: 44): Apr Fr[-1] - Sep 30
Matched (count: 11): Apr Fr[-1] - Sep 30 00:00-24:00
Matched (count: 10): Mar Su[-1]-Oct Su[-1] -1 day: 09:00-20:30; Oct Su[-1]-Mar Su[-1] -1 day: 09:00-17:30
Matched (count: 7): Mo-Fr 10:00-20:30;Sa 09:30-18:00;Su 10:00-17:00;Jan 1 off;Feb Mo[2] off;Easter off;May Mo[-2] off;Jul 1 off;Sep Mo[1] off;Oct Mo[2] off;Dec 25 off
Matched (count: 6): Jul Su[-1] -6 days-Jul Su[-1] 08:00-20:00
[...]

Ref: https://github.com/opening-hours/opening_hours.js/blob/master/regex_search.js

simonpoole commented 6 years ago

@ypid given that it is simply an error that this was not included in the specification, I would think that correcting it, bumping the version and posting on tagging is appropriate.

I would just like to see a grammar beforehand, and probably some guidance on how to handle potential ambiguities, which I believe this leads to at least in simple date selector scenarios (date ranges should be distinguishable from month ranges and therefore shouldn't be an issue), for example

Apr Su[1] 10:00-12:00

This may not be an issue because while it is ambiguous in parsing, it could be that all ambiguous cases end up being functional equivalent. But that needs to be shown.

The difference in testing with my data (~150'000 unique OH strings) is a whooping 53 additional strings that can be parsed (note that I already supported the weekday[n...] element.

ypid commented 6 years ago

Thanks for your feedback! I will do that then later this week.

ypid commented 6 years ago

Done: https://lists.openstreetmap.org/pipermail/tagging/2017-October/033915.html

simonpoole commented 6 years ago

Addressed in https://github.com/simonpoole/OpeningHoursFragment/commit/fb1d82d7ea287166b07fab1eefa970ee62aed66c and https://github.com/simonpoole/OpeningHoursParser/commit/f34a98cc7ca6c712ed41a572e7a7fbfc5c60c1cd