Modify

Opened 15 months ago

Closed 9 months ago

Last modified 6 weeks ago

#18140 closed enhancement (fixed)

Switch to OpeningHoursParser

Reported by: Don-vip Owned by: simon04
Priority: normal Milestone: 20.03
Component: Core Version:
Keywords: opening_hours nashorn sotm19 java15 Cc: Don-vip, ypid23, stoecker, SimonPoole

Description (last modified by simon04)

Oracle is still threatening to remove Nashorn from Java (to make money with their "GraalVM" stuff).
Java will then no longer provide a JavaScript engine, so we won't be able to run our JS libraries anymore (opening_hours and overpass_turbo).

For opening_hours, there's no need to rewrite the lib in Java, as we can simply use Simon's parser:

https://github.com/simonpoole/OpeningHoursParser

Outstanding issues for the migration:

  • Support for collection_times, service_times – not yet supported by OpeningHoursParser?
  • Localized error messages – WIP: Add translation support by simon04 · Pull Request #30 · ​https://github.com/simonpoole/OpeningHoursParser/pull/30
  • The auto-fixing for some cases, such as Mo-Tue, Sa-Su 10.00-20.00, Mo,Tu 04-17
  • (Some tests (mostly of garbage) end with an auto-generated JavaCC exception badtextEncountered " <UNEXPECTED_CHAR> "b "" at line 1, column 1.)
  • Support "wd"
  • Integrate OpeningHoursParser as JAR (via Ivy) or svn:externals (run JavaCC in build.xml)

Some considerations to take into account for "wd":

[00:40:47] <simonpoole> I really need to have a look at generating more descriptive error messages in my parser and add an evaluator but its a month ot two work I suspect
[00:43:07] <simonpoole> don-vip https://github.com/mozilla/rhino
[00:44:00] <don-vip> I know it :) rhino was the javascript engine shipped with java 6/7 until oracle introduces nashorn in 8
[00:44:44] <don-vip> but we won't ship an entire javascript engine just for this. Will probably have to rewrite the library in pure java, which isn't a bad thing
[00:45:54] <simonpoole> well my parser currently has only one small known issue
[00:46:10] <simonpoole> outside of that in implements the full spec
[00:47:36] <don-vip> simonpoole do you have a link to your parser?
[00:48:03] <simonpoole> https://github.com/simonpoole/OpeningHoursParser
[00:49:25] <don-vip> awesome!
[00:49:41] <don-vip> so no need to rewrite the JS library, you made my day Simon
[00:50:18] <simonpoole> it -is- a bit stricter than ypids in that some of the more abstruse deviations from the spec are not handled and some rarely used stuff wd isn't supported but that would be easy to add
[00:50:51] <don-vip> ok, good to know
[00:51:24] <simonpoole> just to be clear wd is not in the spec
[00:52:08] <don-vip> by wd do you mean weekdays or the literal "wd"?
[00:52:46] <simonpoole> the literal "wd" used as an abbrev for Mo-Fr
[00:52:54] <don-vip> ok
[00:53:15] <don-vip> well it's easy to detect it as an error and suggest Mo-Fr instead
[00:53:15] <simonpoole> the thing is that it is not really clear if weekday definition is actually univerally the same
[00:53:24] <don-vip> ah
[00:53:29] <don-vip> good point
[00:54:39] <simonpoole> so really it should be added to the spec and clearly defined
[00:54:58] <petschge> that might be fun in israel or some muslimic countries
[00:55:07] <simonpoole> exactly

Attachments (1)

18140.patch (24.6 KB) - added by simon04 9 months ago.

Download all attachments as: .zip

Change History (61)

comment:1 Changed 15 months ago by Don-vip

Keywords: nashorn sotm19 added

comment:2 Changed 13 months ago by ypid23

Hey guys, that is quite a situation with Oracle. I have not played with ​https://github.com/simonpoole/OpeningHoursParser myself yet but I also see it as a viable option as opening_hours validator. I agree with the concerns from simonpoole about the error messages. I see this as really one of the strong parts of opening_hours.js. I invested days where I mapped/corrected real live opening_hours values and for every single value, I made sure that the library gives meaningful/helpful warnings/errors and help how to correct it. It can even correct most errors for you. All of that is used by JOSM for a long time. I guess when we switch to OpeningHoursParser, users would start to miss those features.

I am not sure what the policies of JOSM are and independent of that I would also not like it, but in terms of usability it might be worth considering me hosting an API endpoint that does the validation for JOSM and we move the calls to opening_hours.js out of JOSM into a NodeJS thing I could host. I do similar things already (ref: https://openingh.openstreetmap.de/api). This could also be made optional if the user does not have Internet or if opting out. I think this would be the best option unless Simon can invest those one-two+ months to get the library up on that area. It also would be an answer to Oracle that we as hackers just find a simple workaround for their business strategy stuff.

Let me know what you think. Also, please feel free to include me in such discussions. I might not have had much time in the last months but I still care deeply about Free Software and OSM.

Last edited 13 months ago by ypid23 (previous) (diff)

comment:3 Changed 9 months ago by Don-vip

Milestone: 20.03

comment:5 in reply to:  2 ; Changed 9 months ago by Don-vip

Replying to ypid23:

in terms of usability it might be worth considering me hosting an API endpoint that does the validation for JOSM and we move the calls to opening_hours.js out of JOSM into a NodeJS thing I could host. I do similar things already (ref: https://openingh.openstreetmap.de/api).

Can we use this existing https://openingh.openstreetmap.de/api endpoint for that?

comment:6 in reply to:  5 Changed 9 months ago by simon04

Replying to Don-vip:

Can we use this existing https://openingh.openstreetmap.de/api endpoint for that?

Not yet. The opening_hours_server.js needs to be extended. Currently, only /api/oh_interpreter is provided which fetches all opening_hours via Overpass for a given bbox. Here's an example: https://github.com/opening-hours/opening_hours_server.js#example-query

comment:7 Changed 9 months ago by simon04

simon04:

Here's a draft implementation: https://github.com/opening-hours/opening_hours_server.js/pull/3


ypid in https://github.com/opening-hours/opening_hours_server.js/pull/3#issuecomment-590117627

Looks good to me, thanks! As you marked it WIP, I just deployed it to openingh.openstreetmap.de for testing but did not merge the PR yet: https://openingh.openstreetmap.de/api/validate?value=PH

comment:8 Changed 9 months ago by simon04

Ticket #18753 has been marked as a duplicate of this ticket.

comment:9 Changed 9 months ago by simon04

Cc: Don-vip ypid23 stoecker added

comment:10 Changed 9 months ago by simon04

The patch attachment:18140.patch​ rewrites the OpeningHourTest to make use of https://github.com/simonpoole/OpeningHoursParser
For testing, compile OpeningHoursParser and put its JAR file in the classpath. The approach looks rather promising. Currently missing / not working in the patch:

  • Support for collection_times, service_times – not supported by OpeningHoursParser?
  • Localized error messages – first step made via https://github.com/simonpoole/OpeningHoursParser/pull/28
  • The auto-fixing for some cases, such as Mo-Tue, Sa-Su 10.00-20.00, Mo,Tu 04-17
  • Some tests (mostly of garbage) end with an auto-generated JavaCC exception badtextEncountered " <UNEXPECTED_CHAR> "b "" at line 1, column 1.

@team: What do you think? Which way to follow (simonpoole/OpeningHoursParser vs. opening-hours/opening_hours_server.js)?

comment:11 in reply to:  10 ; Changed 9 months ago by Don-vip

Replying to simon04:

  • Support for collection_times, service_times – not supported by OpeningHoursParser?

Surprising. Isn't the syntax the same?

@team: What do you think? Which way to follow (simonpoole/OpeningHoursParser vs. opening-hours/opening_hours_server.js)?

OpeningHoursParser first. We need to keep an offline validator, and Nashorn is going to disappear very soon (in Java 15 almost certainly).

We can see next if there's an interest to provide an additional online feature with opening_hours_server.js.

comment:12 Changed 9 months ago by Don-vip

Keywords: java15 added

comment:13 in reply to:  11 Changed 9 months ago by simon04

Replying to Don-vip:

Surprising. Isn't the syntax the same?

collection_times allows to specify points in time w/o a range (such as Mo-Fr 17:30).

comment:14 Changed 9 months ago by simon04

Add translation support by simon04 · Pull Request #30 · https://github.com/simonpoole/OpeningHoursParser/pull/30

comment:15 Changed 9 months ago by simon04

Owner: changed from team to simon04
Status: newassigned

comment:16 Changed 9 months ago by simon04

Description: modified (diff)

comment:17 Changed 9 months ago by simon04

Description: modified (diff)

Concerning "Wd": `Wd is used as a symbol for describing the syntax in https://wiki.openstreetmap.org/wiki/Key:opening_hours, but it is not an allowed value. Also, "wd" is negligible concerning the usage statistics: https://taginfo.openstreetmap.org/keys/opening_hours#values

Changed 9 months ago by simon04

Attachment: 18140.patch added

comment:18 in reply to:  10 Changed 9 months ago by simon04

Replying to simon04:

  • Support for collection_times, service_times – not supported by OpeningHoursParser?

I was wrong – collection_times are in fact (already) supported by OpeningHoursParser

WIP

  • The auto-fixing for some cases, such as Mo-Tue, Sa-Su 10.00-20.00, Mo,Tu 04-17

Taken into account in the amended patch.

So we just have to settle i18n and integrate OpeningHoursParser in the build process.

comment:20 Changed 9 months ago by simon04

In 15977/josm:

see #16860, see #18140 - Setup Apache Ivy (patch by wiktorn, modified)

comment:21 Changed 9 months ago by simon04

Resolution: fixed
Status: assignedclosed

In 15978/josm:

fix #18140 - Switch to OpeningHoursParser

comment:22 Changed 9 months ago by Don-vip

The inclusion of OpeningHoursParser raises:

comment:23 Changed 9 months ago by Don-vip

I need to update the server makefile to reuse the ant build. I will do it tomorrow, so there won't be any nightly build tonight.

comment:24 Changed 9 months ago by simon04

In 15992/josm:

see #18140 - fix ant javadoc

comment:25 Changed 9 months ago by simon04

The Spotbugs warnings relate to classes generated by JavaCC. Can we simply ignore those?

comment:26 in reply to:  23 Changed 9 months ago by skyper

Replying to Don-vip:

I need to update the server makefile to reuse the ant build. I will do it tomorrow, so there won't be any nightly build tonight.

They were build and are not working, see #18833

comment:27 Changed 9 months ago by gaben

Webstart also stopped working.

Last edited 9 months ago by gaben (previous) (diff)

comment:28 Changed 9 months ago by Don-vip

In 16001/josm:

see #18140 - optimize build

comment:29 Changed 9 months ago by Don-vip

In 16002/josm:

see #18140 - refactor build

comment:30 Changed 9 months ago by Don-vip

Server makefile fixed with r16002.

comment:31 Changed 9 months ago by Don-vip

Now I've broken all tests \o/

comment:32 Changed 9 months ago by Don-vip

In 16004/josm:

see #18140 - add nodist folder

comment:33 Changed 9 months ago by Don-vip

In 16005/josm:

see #18140 - add native folder

comment:34 Changed 9 months ago by Don-vip

In 16006/josm:

see #18140 - reorganization of data(_nodist), images(_nodist), styles(_nodist), IDE and native files in a more practical file tree.

  • Everything belonging to the jar is now in resources (data, images, styles)
  • Everything not belonging to the jar is now in nodist (data, images, styles)
  • Everything related to OS native functions is now in native (linux, macosx, windows)
  • Everything related to an IDE is now in ide (eclipse, netbeans)

comment:35 Changed 9 months ago by Don-vip

In 16007/josm:

see #18140 - fix build errors

comment:36 in reply to:  34 ; Changed 9 months ago by skyper

Replying to Don-vip:

In 16006/josm:

see #18140 - reorganization of data(_nodist), images(_nodist), styles(_nodist), IDE and native files in a more practical file tree.

  • Everything belonging to the jar is now in resources (data, images, styles)
  • Everything not belonging to the jar is now in nodist (data, images, styles)
  • Everything related to OS native functions is now in native (linux, macosx, windows)
  • Everything related to an IDE is now in ide (eclipse, netbeans)

So all icon pathes in the wiki need to be adjusted.

comment:37 in reply to:  36 Changed 9 months ago by stoecker

Replying to skyper:

Replying to Don-vip:

In 16006/josm:

see #18140 - reorganization of data(_nodist), images(_nodist), styles(_nodist), IDE and native files in a more practical file tree.

  • Everything belonging to the jar is now in resources (data, images, styles)
  • Everything not belonging to the jar is now in nodist (data, images, styles)
  • Everything related to OS native functions is now in native (linux, macosx, windows)
  • Everything related to an IDE is now in ide (eclipse, netbeans)

So all icon pathes in the wiki need to be adjusted.

We probably can do this semi-automatic.

comment:38 in reply to:  34 Changed 9 months ago by simon04

Let's continue discussing the reorganization in #18845. This ticket is about OpeningHoursParser and fixed.

comment:39 Changed 9 months ago by simon04

In 16011/josm:

see #16860, see #18140 - Restrict SpotBugs to org/openstreetmap/josm

comment:40 Changed 9 months ago by skyper

ypid23 on #18807 comment 10:

Should Fr-We,PH be changed to PH,Fr-We?

I would say the spec is too restrictive here. I updated it. For details refer to: https://wiki.openstreetmap.org/wiki/Talk:Key:opening_hours/specification#Spec_to_restrictive._PH.2CMo-Fr_allowed_but_Mo-Fr.2CPH_is_not.

@simon04
I get Opening hours syntax - Holiday after weekday at line 1, column 10 for opening_hours=Mo-Su,PH 10:00-21:30

  • the new parser is too strict.
Last edited 9 months ago by skyper (previous) (diff)

comment:41 Changed 9 months ago by skyper

simon04 on #18807 comment 6:

The new implementation via #18140 currently triggers the following error:

Encountered " <HOLIDAYS> "PH "" at line 1, column 26.
Was expecting:
    <EOF> 

Not really understandable and without auto fix for most user unusable !

Are there any plans for human messages and auto fix ?

comment:42 Changed 9 months ago by stoecker

Resolution: fixed
Status: closedreopened

The released JAR contains a lot of stuff which should not be there.

     4477  2020-03-03 21:01   ch/poole/openinghoursparser/Holiday.java
    19566  2020-03-03 21:01   ch/poole/openinghoursparser/WeekRange.html
     4510  2020-03-03 21:01   ch/poole/openinghoursparser/WeekRange.java
    18523  2020-03-03 21:01   ch/poole/openinghoursparser/Holiday.html
     7457  2020-03-03 21:01   ch/poole/openinghoursparser/Util.java
     5236  2020-03-03 21:01   ch/poole/openinghoursparser/RuleModifier.java
    16504  2020-03-03 21:01   ch/poole/openinghoursparser/Event.html
     2585  2020-03-03 21:01   ch/poole/openinghoursparser/VarDate.java
    35908  2020-03-03 21:01   ch/poole/openinghoursparser/Rule.html
    19141  2020-03-03 21:01   ch/poole/openinghoursparser/YearRange.html
       97  2020-03-03 21:01   ch/poole/openinghoursparser/package-info.java
     2600  2020-03-03 21:01   ch/poole/openinghoursparser/Event.java
    34275  2020-03-03 21:01   ch/poole/openinghoursparser/DateWithOffset.html
    14397  2020-03-03 21:01   ch/poole/openinghoursparser/Rule.java
    22106  2020-03-03 21:01   ch/poole/openinghoursparser/WeekDayRange.html
    17109  2020-03-03 21:01   ch/poole/openinghoursparser/VariableTime.html
     3918  2020-03-03 21:01   ch/poole/openinghoursparser/VariableTime.java
    20523  2020-03-03 21:01   ch/poole/openinghoursparser/UnitTest.java
     8953  2020-03-03 21:01   ch/poole/openinghoursparser/DataTest.java
     1228  2020-03-03 21:01   ch/poole/openinghoursparser/I18n.java
     1290  2020-03-03 21:01   ch/poole/openinghoursparser/Messages.properties
     1380  2020-03-03 21:01   ch/poole/openinghoursparser/Messages_de.properties
     5670  2020-03-03 21:01   ch/poole/openinghoursparser/DateRange.java
    15228  2020-03-03 21:01   ch/poole/openinghoursparser/VarDate.html
    24841  2020-03-03 21:01   ch/poole/openinghoursparser/TimeSpan.html
    10878  2020-03-03 21:01   ch/poole/openinghoursparser/I18n.html
    11283  2020-03-03 21:01   ch/poole/openinghoursparser/OpeningHoursParseException.html
    17400  2020-03-03 21:01   ch/poole/openinghoursparser/RuleModifier.Modifier.html
    18372  2020-03-03 21:01   ch/poole/openinghoursparser/RuleModifier.html
     3371  2020-03-03 21:01   ch/poole/openinghoursparser/Nth.java
    11339  2020-03-03 21:01   ch/poole/openinghoursparser/DateWithOffset.java
     3470  2020-03-03 21:01   ch/poole/openinghoursparser/OpeningHoursParseException.java
    21338  2020-03-03 21:01   ch/poole/openinghoursparser/Month.html
    19373  2020-03-03 21:01   ch/poole/openinghoursparser/DateRange.html
      193  2020-03-03 21:01   ch/poole/openinghoursparser/Copy.java
     8937  2020-03-03 21:01   ch/poole/openinghoursparser/Copy.html
    14767  2020-03-03 21:01   ch/poole/openinghoursparser/Util.html
    16862  2020-03-03 21:01   ch/poole/openinghoursparser/Nth.html
      638  2020-03-03 21:01   ch/poole/openinghoursparser/Element.java
     3633  2020-03-03 21:01   ch/poole/openinghoursparser/package-frame.html
     9942  2020-03-03 21:01   ch/poole/openinghoursparser/DataTest.html
     3527  2020-03-03 21:01   ch/poole/openinghoursparser/Month.java
     4715  2020-03-03 21:01   ch/poole/openinghoursparser/YearRange.java
     2616  2020-03-03 21:01   ch/poole/openinghoursparser/WeekDay.java
    17876  2020-03-03 21:01   ch/poole/openinghoursparser/WeekDay.html
    11879  2020-03-03 21:01   ch/poole/openinghoursparser/package-summary.html
     9386  2020-03-03 21:01   ch/poole/openinghoursparser/package-tree.html
    15538  2020-03-03 21:01   ch/poole/openinghoursparser/UnitTest.html
    29027  2020-03-03 21:01   ch/poole/openinghoursparser/OpeningHoursParser.jj
     6852  2020-03-03 21:01   ch/poole/openinghoursparser/TimeSpan.java
     6926  2020-03-03 21:01   ch/poole/openinghoursparser/WeekDayRange.java
    13954  2020-03-03 21:01   ch/poole/openinghoursparser/Holiday.Type.html

comment:43 Changed 9 months ago by simon04

In 16016/josm:

see #18140 - Do not include OpeningHoursParser-{sources,javadoc}.jar

comment:44 Changed 9 months ago by Don-vip

In 16020/josm:

see #16860, see #18140 - better way to restrict SpotBugs analysis scope (without error about missing classes)

See https://github.com/spotbugs/spotbugs/blob/4.0.0/spotbugs-ant/src/main/java/edu/umd/cs/findbugs/anttask/FindBugsTask.java

comment:45 Changed 9 months ago by simon04

Resolution: fixed
Status: reopenedclosed

comment:46 Changed 9 months ago by skyper

See #18892 for features which got lost in transition.

comment:47 Changed 9 months ago by Don-vip

Cc: SimonPoole added

comment:48 Changed 9 months ago by simon04

In 16085/josm:

see #18140 - Remove obsolete OpeningHoursParser.CheckMode

comment:49 Changed 9 months ago by simon04

In 16086/josm:

see #18140 - Get rid of OpeningHoursParser.OpeningHoursTestError

comment:50 Changed 9 months ago by simon04

In 16114/josm:

see #18140 - Update OpeningHoursParser to 0.21.1

Updated translations, removed annotations as a runtime dependency.

comment:52 Changed 9 months ago by simon04

In 16116/josm:

see #18140 - ant dist-optimized: fix "can't find referenced class org.jetbrains.annotations."

comment:53 Changed 9 months ago by mdk

Alternatively the "org.jetbrains.annotations.Nullable" could be replaced by "javax.validation.constraints"

comment:54 in reply to:  52 ; Changed 9 months ago by Don-vip

Replying to simon04:

In 16116/josm:

see #18140 - ant dist-optimized: fix "can't find referenced class org.jetbrains.annotations."

Does the optimized jar still work?

comment:55 in reply to:  54 Changed 9 months ago by simon04

Replying to Don-vip:

Does the optimized jar still work?

Yes, those interfaces are not required for runtime. There's no functionality associated with them.

Verified: node amenity=restaurant opening_hours=Mo 7-19 correctly reports a validator warning.

comment:56 Changed 9 months ago by simon04

In 16140/josm:

see #18140 - build.xml: use ${resources.dir}

comment:57 Changed 8 months ago by simon04

In 16234/josm:

see #18140 - TagInfoExtract: fix icon_url

comment:58 Changed 6 weeks ago by simon04

Nashorn is coming back:

2020-10-11 [nashorn-dev] Standalone Nashorn is coming for Java 15+ – https://mail.openjdk.java.net/pipermail/nashorn-dev/2020-October/007557.html

comment:59 Changed 6 weeks ago by Don-vip

I wasn't expecting that. Oracle does really a terrible communication job... I'm still glad of all the work you did to switch to Java-based implementations :)

comment:60 Changed 6 weeks ago by GerdP

+1

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain simon04.
as The resolution will be set.
The resolution will be deleted.

Add Comment


E-mail address and name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.