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.

Sign up for GitHub

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

Jump to bottom

Fix a single weekday grammar callback #4357

Merged
merged 1 commit into from
Jul 31, 2017
Merged

Fix a single weekday grammar callback #4357

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions include/util/opening_hours.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ namespace util
{

// Helper classes for "opening hours" format http://wiki.openstreetmap.org/wiki/Key:opening_hours
// Grammar https://wiki.openstreetmap.org/wiki/Key:opening_hours/specification
// Supported simplified features in CheckOpeningHours:
// - Year/Month/Day ranges
// - Weekday ranges
Expand Down
6 changes: 3 additions & 3 deletions src/util/opening_hours.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -218,9 +218,9 @@ struct opening_hours_grammar : qi::grammar<Iterator, Skipper, std::vector<Openin
weekday_sequence = (weekday_range % ',')[ph::bind(&OpeningHours::weekdays, _r1) = _1];

weekday_range
= wday[_a = _1, _b = _1]
>> -(('-' >> wday[_b = _1])
| ('[' >> (nth_entry % ',') >> ']' >> -day_offset))
= (wday[_a = _1, _b = _1]
Copy link

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?

Copy link
Contributor Author

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.

Copy link

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.

>> -(('-' >> wday[_b = _1])
| ('[' >> (nth_entry % ',') >> ']' >> -day_offset)))
[_val = ph::construct<OpeningHours::WeekdayRange>(_a, _b)]
;

Expand Down
94 changes: 54 additions & 40 deletions unit_tests/util/opening_hours_parsing.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,11 @@

BOOST_AUTO_TEST_SUITE(opening_hours)

// Some tests from https://www.netzwolf.info/en/cartography/osm/time_domain/explanation

using osrm::util::ParseOpeningHours;
using osrm::util::CheckOpeningHours;

// convert a string representation of time to a tm structure
struct tm time(const char *str)
{
Expand All @@ -21,8 +26,6 @@ struct tm time(const char *str)

BOOST_AUTO_TEST_CASE(check_opening_hours_grammar)
{
using osrm::util::ParseOpeningHours;

const std::string opening_hours[] = {
"Apr 10-Jun 15",
"Apr 10-15 off",
Expand All @@ -45,7 +48,9 @@ BOOST_AUTO_TEST_CASE(check_opening_hours_grammar)
"Tu,Th 16:00-20:00",
"2016 Feb-2017 Dec",
"2016-2017",
"Mo,Tu,Th,Fr 12:00-18:00;Sa 12:00-17:00; Th[3] off; Th[-1] off"};
"Mo,Tu,Th,Fr 12:00-18:00;Sa 12:00-17:00; Th[3] off; Th[-1] off",
"Sep 15+Sa-Oct Su[1]; Oct 01-03" // Oktoberfest
};

for (auto &input : opening_hours)
{
Expand All @@ -55,8 +60,6 @@ BOOST_AUTO_TEST_CASE(check_opening_hours_grammar)

BOOST_AUTO_TEST_CASE(check_opening_hours_grammar_incorrect_correct)
{
using osrm::util::ParseOpeningHours;

const std::pair<std::string, std::string> opening_hours[] = {
{"7/8-23", "Mo-Su 08:00-23:00"},
{"0600-1800", "06:00-18:00"},
Expand Down Expand Up @@ -88,8 +91,6 @@ BOOST_AUTO_TEST_CASE(check_opening_hours_grammar_incorrect_correct)

BOOST_AUTO_TEST_CASE(check_rules_grouping)
{
using osrm::util::ParseOpeningHours;

const auto &result1 = ParseOpeningHours("Su-Th 11:00-03:00, Fr-Sa 11:00-05:00");
BOOST_REQUIRE_EQUAL(result1.size(), 2);
BOOST_CHECK_EQUAL(result1.at(0).times.size(), 1);
Expand All @@ -108,9 +109,6 @@ BOOST_AUTO_TEST_CASE(check_rules_grouping)

BOOST_AUTO_TEST_CASE(check_opening_hours_time_and_weekday)
{
using osrm::util::ParseOpeningHours;
using osrm::util::CheckOpeningHours;

const auto &opening_hours = ParseOpeningHours("Mo-Fr 08:30-20:00");
BOOST_CHECK_EQUAL(CheckOpeningHours(opening_hours, time("Wed, 14 Dec 2016 12:32:00")), true);
BOOST_CHECK_EQUAL(CheckOpeningHours(opening_hours, time("Wed, 14 Dec 2016 21:32:00")), false);
Expand All @@ -120,13 +118,56 @@ BOOST_AUTO_TEST_CASE(check_opening_hours_time_and_weekday)
BOOST_CHECK_EQUAL(CheckOpeningHours(opening_hours, time("Mon, 19 Dec 2016 08:29:59")), false);
BOOST_CHECK_EQUAL(CheckOpeningHours(opening_hours, time("Fri, 23 Dec 2016 19:59:59")), true);
BOOST_CHECK_EQUAL(CheckOpeningHours(opening_hours, time("Fri, 23 Dec 2016 20:00:00")), false);

const auto &opening_hours2 = ParseOpeningHours("Su 00:00-23:59");
BOOST_CHECK_EQUAL(CheckOpeningHours(opening_hours2, time("Sat, 29 Jul 2017 12:57:51")), false);
BOOST_CHECK_EQUAL(CheckOpeningHours(opening_hours2, time("Sun, 30 Jul 2017 19:47:51")), true);
BOOST_CHECK_EQUAL(CheckOpeningHours(opening_hours2, time("Sun, 30 Jul 2017 23:59:30")), false);
BOOST_CHECK_EQUAL(CheckOpeningHours(opening_hours2, time("Mon, 31 Jul 2017 05:52:22")), false);
BOOST_CHECK_EQUAL(CheckOpeningHours(opening_hours2, time("Mon, 31 Jul 2017 00:00:00")), false);

const auto &opening_hours3 = ParseOpeningHours("Su 00:00-24:00");
BOOST_CHECK_EQUAL(CheckOpeningHours(opening_hours3, time("Sat, 29 Jul 2017 23:59:59")), false);
BOOST_CHECK_EQUAL(CheckOpeningHours(opening_hours3, time("Sun, 30 Jul 2017 19:47:51")), true);
BOOST_CHECK_EQUAL(CheckOpeningHours(opening_hours3, time("Sun, 30 Jul 2017 23:59:30")), true);
BOOST_CHECK_EQUAL(CheckOpeningHours(opening_hours3, time("Mon, 31 Jul 2017 00:00:00")), false);
}

BOOST_AUTO_TEST_CASE(check_opening_hours_year_month)
BOOST_AUTO_TEST_CASE(check_opening_hours_only_weekdays)
{
using osrm::util::ParseOpeningHours;
using osrm::util::CheckOpeningHours;
const auto &opening_hours = ParseOpeningHours("Su");
BOOST_CHECK_EQUAL(CheckOpeningHours(opening_hours, time("Sat, 29 Jul 2017 23:59:59")), false);
BOOST_CHECK_EQUAL(CheckOpeningHours(opening_hours, time("Sun, 30 Jul 2017 19:47:51")), true);
BOOST_CHECK_EQUAL(CheckOpeningHours(opening_hours, time("Sun, 30 Jul 2017 23:59:30")), true);
BOOST_CHECK_EQUAL(CheckOpeningHours(opening_hours, time("Mon, 31 Jul 2017 00:00:00")), false);

const auto &opening_hours2 = ParseOpeningHours("Mo-Sa");
BOOST_CHECK_EQUAL(CheckOpeningHours(opening_hours2, time("Fri, 28 Jul 2017 09:11:11")), true);
BOOST_CHECK_EQUAL(CheckOpeningHours(opening_hours2, time("Sat, 29 Jul 2017 23:59:59")), true);
BOOST_CHECK_EQUAL(CheckOpeningHours(opening_hours2, time("Sun, 30 Jul 2017 19:47:51")), false);
BOOST_CHECK_EQUAL(CheckOpeningHours(opening_hours2, time("Sun, 30 Jul 2017 23:59:30")), false);
BOOST_CHECK_EQUAL(CheckOpeningHours(opening_hours2, time("Mon, 31 Jul 2017 00:00:00")), true);
BOOST_CHECK_EQUAL(CheckOpeningHours(opening_hours2, time("Tue, 01 Aug 2017 10:24:31")), true);

const auto &opening_hours3 = ParseOpeningHours("Sa-Mo");
BOOST_CHECK_EQUAL(CheckOpeningHours(opening_hours3, time("Fri, 28 Jul 2017 09:11:11")), false);
BOOST_CHECK_EQUAL(CheckOpeningHours(opening_hours3, time("Sat, 29 Jul 2017 23:59:59")), true);
BOOST_CHECK_EQUAL(CheckOpeningHours(opening_hours3, time("Sun, 30 Jul 2017 19:47:51")), true);
BOOST_CHECK_EQUAL(CheckOpeningHours(opening_hours3, time("Sun, 30 Jul 2017 23:59:30")), true);
BOOST_CHECK_EQUAL(CheckOpeningHours(opening_hours3, time("Mon, 31 Jul 2017 00:00:00")), true);
BOOST_CHECK_EQUAL(CheckOpeningHours(opening_hours3, time("Tue, 01 Aug 2017 10:24:31")), false);

const auto &opening_hours4 = ParseOpeningHours("Mo-We,Sa");
BOOST_CHECK_EQUAL(CheckOpeningHours(opening_hours4, time("Fri, 28 Jul 2017 09:11:11")), false);
BOOST_CHECK_EQUAL(CheckOpeningHours(opening_hours4, time("Sat, 29 Jul 2017 23:59:59")), true);
BOOST_CHECK_EQUAL(CheckOpeningHours(opening_hours4, time("Sun, 30 Jul 2017 19:47:51")), false);
BOOST_CHECK_EQUAL(CheckOpeningHours(opening_hours4, time("Sun, 30 Jul 2017 23:59:30")), false);
BOOST_CHECK_EQUAL(CheckOpeningHours(opening_hours4, time("Mon, 31 Jul 2017 00:00:00")), true);
BOOST_CHECK_EQUAL(CheckOpeningHours(opening_hours4, time("Tue, 01 Aug 2017 10:24:31")), true);
}

BOOST_AUTO_TEST_CASE(check_opening_hours_year_month)
{
const auto &opening_hours = ParseOpeningHours("2016 Feb-2017 Dec");
BOOST_CHECK_EQUAL(CheckOpeningHours(opening_hours, time("Sun, 31 Jan 2016 12:00:00")), false);
BOOST_CHECK_EQUAL(CheckOpeningHours(opening_hours, time("Wed, 14 Dec 2016 12:32:00")), true);
Expand All @@ -136,9 +177,6 @@ BOOST_AUTO_TEST_CASE(check_opening_hours_year_month)

BOOST_AUTO_TEST_CASE(check_opening_hours_year_monthday)
{
using osrm::util::ParseOpeningHours;
using osrm::util::CheckOpeningHours;

const auto &opening_hours = ParseOpeningHours("2019 Apr 10-Jun 15");
BOOST_CHECK_EQUAL(CheckOpeningHours(opening_hours, time("Sun, 16 Apr 2017 17:59:00")), false);
BOOST_CHECK_EQUAL(CheckOpeningHours(opening_hours, time("Tue, 09 Apr 2019 23:59:00")), false);
Expand All @@ -150,9 +188,6 @@ BOOST_AUTO_TEST_CASE(check_opening_hours_year_monthday)

BOOST_AUTO_TEST_CASE(check_opening_hours_year_monthday_time_and_weekday)
{
using osrm::util::ParseOpeningHours;
using osrm::util::CheckOpeningHours;

const auto &opening_hours = ParseOpeningHours("2017 Feb-May Sa-Tu 08:00-18:00");
BOOST_CHECK_EQUAL(CheckOpeningHours(opening_hours, time("Mon, 12 Dec 2016 12:32:00")), false);
BOOST_CHECK_EQUAL(CheckOpeningHours(opening_hours, time("Tue, 31 Jan 2017 23:59:00")), false);
Expand All @@ -167,9 +202,6 @@ BOOST_AUTO_TEST_CASE(check_opening_hours_year_monthday_time_and_weekday)

BOOST_AUTO_TEST_CASE(check_opening_hours_time_plus)
{
using osrm::util::ParseOpeningHours;
using osrm::util::CheckOpeningHours;

const auto &opening_hours = ParseOpeningHours("08:00+");
BOOST_CHECK_EQUAL(CheckOpeningHours(opening_hours, time("Mon, 12 Dec 2016 07:59:00")), false);
BOOST_CHECK_EQUAL(CheckOpeningHours(opening_hours, time("Mon, 12 Dec 2016 08:00:00")), true);
Expand All @@ -178,9 +210,6 @@ BOOST_AUTO_TEST_CASE(check_opening_hours_time_plus)

BOOST_AUTO_TEST_CASE(check_opening_hours_off)
{
using osrm::util::ParseOpeningHours;
using osrm::util::CheckOpeningHours;

const auto &opening_hours = ParseOpeningHours("08:00-20:00; 12:00-14:30 off; 12:30-13:30 open");
BOOST_CHECK_EQUAL(CheckOpeningHours(opening_hours, time("Mon, 12 Dec 2016 07:00:00")), false);
BOOST_CHECK_EQUAL(CheckOpeningHours(opening_hours, time("Mon, 12 Dec 2016 08:00:00")), true);
Expand All @@ -193,9 +222,6 @@ BOOST_AUTO_TEST_CASE(check_opening_hours_off)

BOOST_AUTO_TEST_CASE(check_opening_hours_overnight_multiple_times)
{
using osrm::util::ParseOpeningHours;
using osrm::util::CheckOpeningHours;

const auto &opening_hours = ParseOpeningHours("20 08:00-10:00,20:00-03:00");
BOOST_CHECK_EQUAL(CheckOpeningHours(opening_hours, time("Tue, 20 Dec 2016 02:00:00")), false);
BOOST_CHECK_EQUAL(CheckOpeningHours(opening_hours, time("Tue, 20 Dec 2016 07:00:00")), false);
Expand All @@ -210,19 +236,13 @@ BOOST_AUTO_TEST_CASE(check_opening_hours_overnight_multiple_times)

BOOST_AUTO_TEST_CASE(check_opening_hours_overnight_weekdays)
{
using osrm::util::ParseOpeningHours;
using osrm::util::CheckOpeningHours;

const auto &opening_hours = ParseOpeningHours("Mo-Fr 20:00-03:00");
BOOST_CHECK_EQUAL(CheckOpeningHours(opening_hours, time("Mon, 12 Dec 2016 02:00:00")), false);
BOOST_CHECK_EQUAL(CheckOpeningHours(opening_hours, time("Sat, 17 Dec 2016 02:00:00")), true);
}

BOOST_AUTO_TEST_CASE(check_opening_hours_overnight_days)
{
using osrm::util::ParseOpeningHours;
using osrm::util::CheckOpeningHours;

const auto &opening_hours = ParseOpeningHours("Dec 12-17 20:00-03:00");
BOOST_CHECK_EQUAL(CheckOpeningHours(opening_hours, time("Mon, 12 Dec 2016 02:00:00")), false);
BOOST_CHECK_EQUAL(CheckOpeningHours(opening_hours, time("Mon, 12 Dec 2016 20:00:00")), true);
Expand All @@ -234,9 +254,6 @@ BOOST_AUTO_TEST_CASE(check_opening_hours_overnight_days)

BOOST_AUTO_TEST_CASE(check_opening_hours_extended_hours_overlapping)
{
using osrm::util::ParseOpeningHours;
using osrm::util::CheckOpeningHours;

const auto &opening_hours = ParseOpeningHours("Dec 20 08:00-44:00");
BOOST_CHECK_EQUAL(CheckOpeningHours(opening_hours, time("Mon, 19 Dec 2016 07:00:00")), false);
BOOST_CHECK_EQUAL(CheckOpeningHours(opening_hours, time("Tue, 20 Dec 2016 07:00:00")), false);
Expand All @@ -256,9 +273,6 @@ BOOST_AUTO_TEST_CASE(check_opening_hours_extended_hours_overlapping)

BOOST_AUTO_TEST_CASE(check_opening_hours_extended_hours_nonoverlapping)
{
using osrm::util::ParseOpeningHours;
using osrm::util::CheckOpeningHours;

const auto &opening_hours = ParseOpeningHours("Dec 20 20:00-32:00");
BOOST_CHECK_EQUAL(CheckOpeningHours(opening_hours, time("Mon, 19 Dec 2016 07:00:00")), false);
BOOST_CHECK_EQUAL(CheckOpeningHours(opening_hours, time("Tue, 20 Dec 2016 07:00:00")), false);
Expand Down