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

It makes sense to split/refactor library into multiple according levels of compliance #143

Open
d1g opened this issue Feb 12, 2016 · 11 comments
Labels
type: feature Introduction of new functionality.

Comments

@d1g
Copy link

d1g commented Feb 12, 2016

70K instances of 523K are just "24/7"

I.e. if I write "if (opening_hours="24/7") { } " my code will get 13% of the data correctly.

There should be

  • ruleset60% (or 70/80) - simplest and most popular cases (win in terms of performance and memory usage). It is a set of rules you can explain to newbies or just slightly more advanced cases.
  • ruleset90% - most cases without support for inconsistent data
  • ruleset95%+ - most sophisticated rule-set supporting all quirks and features in data

While I appreciate efforts to support almost 100% of the data out of box, but performance hit got too big:
1k/sec vs 20k/sec in AMDmi3 version

Another suggestion is to create libraries on top of each other, so you can get 100% opening_hours library simply by chaining calls to 3 libraries rs1 -> rs2 -> rs3 without overhead of common rules in (rs1, rs2) (rs1, rs3) (rs2, rs3)

@ypid ypid added the type: feature Introduction of new functionality. label Feb 12, 2016
@ypid ypid added this to the world conquest milestone Feb 12, 2016
@ypid
Copy link
Member

ypid commented Feb 12, 2016

Sounds reasonable but I don’t intent to work on this for the foreseeable future as there are more important bugs/features left to do.

@ypid
Copy link
Member

ypid commented Apr 13, 2016

This is related to #150. I will keep those wishes in mind when I eventually find the time to start working on #136 and make it configurable how high the coverage should be and maybe provide support to enable/disable individual features when targeting a platform thus making it universal. If you need a small version of this library for your microcontroller project it should be possible with #136 to target C++ for example and generate a limited version.

@d1g
Copy link
Author

d1g commented Apr 24, 2016

@ypid current spec page is imprecise or incomplete

I tried to follow it as close as I could, but it led me to several unclear cases quite fast: unclear rules about additional_rule_separator and some rare syntax examples

I don't have numbers how much of valid OH library inputs it treats as valid and how much invalid OH inputs it treats as valid (false negatives due less semantic parsing).

IMO it is faster to prototype/refactor using 300L long grammar than 7.2K long implementation.

@ypid
Copy link
Member

ypid commented Apr 24, 2016

Awesome that you are interested in this subject 👍 and I find you approach of using ANTLR very interesting. I did not stumble across this project yet but I will definitely check it out. I was more looking into older parser generators like Bison and some from the Haxe world but I have not yet started to seriously work on #136. ANTLR seems to support the common programming languages which makes it suitable for the job.

current spec page is imprecise or incomplete

You are right. There are still open issues … But I guess you already know how complex this subject is and that it will probably be a life-time job at least when it should support all countries/cultures properly.

About the problem with <additional_rule_separator>. I would say the real problem is still the double use of the , token for this (see #86). Other then that I fine with additional rules.

Thanks for making me a contributor by the way 😉. We can definitely move the repository over to the opening-hours organization if you like so that others can find it more easily.

@d1g
Copy link
Author

d1g commented Apr 24, 2016

@ypid, that's insane we actually have support for sunset-related events; IMO they should be separate libs (sunset lib, public hours lib) so more people can reach them, not just OSM-focused folks.

If token is ambiguous, we should change syntax to less ambiguous ('.' or '&' symbol is unused AFAIK).

PS. I just tried to move repo but GitHub said: "You don’t have admin rights to opening-hours"

@ypid
Copy link
Member

ypid commented Apr 24, 2016

If token is ambiguous, we should change syntax to less ambiguous ('.' or '&' symbol is unused AFAIK).

I already discussed this here #53 (comment). I am not against establishing a better token for this. My suggestion would be &&. You are welcome to create a proposal for this and get it standardized if the OSM community can agree on this.

that's insane we actually have support for sunset-related events; IMO they should be separate libs (sunset lib, public hours lib) so more people can reach them, not just OSM-focused folks.

I have focused on OSM and the uses for this lib I know of are all OSM related. Support for non-OSM use is not my focus but supporting it on the other hand does not hurt either.

https://github.com/opening-hours

Hm, can you join the organization, I invited you? If that does not work, you can temporarly transfer the repo to ypid as transferring between user accounts is easier.

@ypid
Copy link
Member

ypid commented Apr 24, 2016

@d1g Thanks, I moved your repository to https://github.com/opening-hours/opening_hours_grammar.

@d1g
Copy link
Author

d1g commented Apr 24, 2016

I have focused on OSM and the uses for this lib I know of are all OSM related. Support for non-OSM use is not my focus but supporting it on the other hand does not hurt either.

This feature was requested by others, it makes sense to re-use good solutions, rather than waste time re-implementing them:
https://www.google.com/search?q=site:stackoverflow.com+public+holidays

I'm not fan of PHP, but http://kayaposoft.com/enrico/ is still active (from this question)

There also library that calculates "day/night/dusk/etc" states for you, but I forgot name of it.

"This repository is being transferred to @ypid" - https://github.com/d1g/opening_hours_grammar/settings

@ypid
Copy link
Member

ypid commented Apr 24, 2016

There also library that calculates "day/night/dusk/etc" states for you, but I forgot name of it.

We already got that covered: https://github.com/ypid/suncalc

This feature was requested by others, it makes sense to re-use good solutions, rather than waste time re-implementing them

I absolutely agree. I did check every module on npm a few years ago if there way some lib that could be used. There was none. I would also like to move PH and SH stuff out of the main lib to make it reusable. As there is a fair number of supported PHs by now, this could be interesting. See #71. The difficult part is to define a useful data format and decide what code/API to use.

@ypid
Copy link
Member

ypid commented Nov 10, 2016

Seems others have stepping in to help with this issue 😉 SimpleOpeningHours only covers a very small subset of the spec, which is a design goal. This should partly solve this issue. Depending on your needs, you might want to check SimpleOpeningHours. You could then use opening_hours.js as a fallback for all the crazy stuff 😉 Also refer to Other implementations in the README.

@d1g
Copy link
Author

d1g commented Nov 10, 2016

@ypid, personally I wait for implementation that would be suitable for databases

In my complete use case grammar is used (among easy tracing) to insert rules for recurring_event_specs here: http://stackoverflow.com/a/947349 Everything is calculated/precomputed inside db (PostgreSQL). Then "cached" events would be re-used in much heavy-weight tasks.

DB-wise: sunrise/sunset 1. variadic for every day 2. variadic for every object on the Planet - because of 2 they are much more demanding than other rules.

other less relevant links: 1 2 3

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: feature Introduction of new functionality.
Projects
None yet
Development

No branches or pull requests

2 participants