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

Added opening hours validator. #5

Merged
merged 15 commits into from Sep 6, 2014

Conversation

ypid
Copy link
Contributor

@ypid ypid commented Aug 19, 2014

Hi

Nice validation plugin. I tried to add support for opening_hours but I have no idea how to debug it in JOSM. I did not find a error console or something like this. Next thing is that I do not run any logic or scripts on server side to evaluate opening_hours errors. So my approach for this plugin is simply to download all opening_hours in one area and check them with pyopening_hours for problems on the client.

I wrote a proof of concept. If you like it could you please adopt the API of qat_script for this and get it to work in JOSM to get me started?

@ypid
Copy link
Contributor Author

ypid commented Aug 19, 2014

Additionally, the package pyopening_hours should probably be bundled with qat_script to ensure that it is present.

@simone-f
Copy link
Owner

Hi!

I tried to add support for opening_hours but I have no idea how to debug it in JOSM. I did not find a error console or something like this.

Just start JOSM from the terminal.

I wrote a proof of concept. If you like it could you please adopt the API of qat_script for this and get it to work in JOSM to get me started?

The following things should be fixed in your code.

https://github.com/ypid/qat_script/blob/added-opening_hours_validator/tools/data/OpeningHoursValidator/OpeningHoursValidator.py#L19
Change the class name from OpeningHoursValidator to OpeningHoursValidatorTool

https://github.com/ypid/qat_script/blob/added-opening_hours_validator/tools/data/OpeningHoursValidator/OpeningHoursValidator.py#L78
Add an icon name and marker name to every check (you can use a fake name by now).
E.g.

["opening_hours warnings and errors",
"error",
"string used by create_url() to request errors from server",
"icon name",
"marker name"]

Additionally, the package pyopening_hours should probably be bundled with qat_script to ensure that it is present.

I have seen proof_of_concept.py but I couldn't test it (I can't properly install pyopening_hours).
Please, add the module to the PR so I can test it.

Anyhow, if you want to integrate the error detection by pyopening_hours into qat_script you should copy the part from line 54 of proof_of_concept.py to parse_error_file() method of OpeningHoursValidatorTool class (I could do that for you if I could test proof_of_concept.py).
Since you are parsing a json file you might want to take a look to the parsing method for Osmose reports:
https://github.com/simone-f/qat_script/blob/development/tools/data/Osmose/Osmose.py#L228

Hint: uncomment these lines while testing, so that you don't have to restart JOSM every time you modify OpeningHoursValidator.py. Then you can just launch qat_script from "Scripting" menu again to reload qat_script and OpeningHoursValidator tool.
https://github.com/ypid/qat_script/blob/added-opening_hours_validator/qat_script.py#L898

@ypid
Copy link
Contributor Author

ypid commented Aug 20, 2014

Thanks for your answer.

I have seen proof_of_concept.py but I couldn't test it (I can't properly install pyopening_hours).
Please, add the module to the PR so I can test it.

What do you mean by PR?

Just start JOSM from the terminal.

I tried this already, but it seems that not every stack trace makes it to the console. Thats why I asked. It worked most of the times …

* Copied icons from https://github.com/ypid/opening_hours_map/tree/master/img
* Wrote gen_translations to convert translations from https://github.com/ypid/opening_hours.js/blob/master/js/i18n-resources.js
* Generated translations.

Open problems:

* pyopening_hours can not be imported?
* self.isTranslated = True causes crash without error message in the console.
@simone-f
Copy link
Owner

What do you mean by PR?

Sorry, pull request.

I tried this already, but it seems that not every stack trace makes it to the console. Thats why I asked. It worked most of the times …
self.isTranslated = True causes crash without error message in the console.

I noticed the same problem some time ago while executing a part of the code in another thread, maybe while downloading (DownloadTask) or parsing errors (ParseTask); but in this case, the error message shows up on my console:

javax.script.ScriptException: java.util.MissingResourceException: java.util.MissingResourceException: Can't find resource for bundle java.util.PropertyResourceBundle, key w_opening_hours

You should add the translation of the view (the category collecting all your tool's checks) to the ResourceBoundle. For example:
w_opening_hours = Errors and warnings

Another fix.
In this line, change self.title to self.title = 'Opening Hours Validator'.
This will fix the icons not showing.

On a different note, it seems that the data is correclty downloaded from overpass and parsed, since logging says:

DEBUG (OpeningHoursValidatorTool): parseTask.errors: {'filter_error': []}
DEBUG (OpeningHoursValidatorTool): parseTask.tool: <tools.data.OpeningHoursValidator.OpeningHoursValidator.OpeningHoursValidatorTool instance at 0xa>
DEBUG (OpeningHoursValidatorTool): Tag name: Supermarket X
DEBUG (OpeningHoursValidatorTool): Tag name: Parking A
DEBUG (OpeningHoursValidatorTool): Tag name: Restaurant B
...

Open problems:

  • pyopening_hours can not be imported?

Could you please add the modules to the pull request so I can test it?

Run `make` to move it to the correct location.

  File "$HOME/Projekte/src/osm/qat_script/pyopening_hours/OpeningHours.py", line 16, in <module>
    from com.xhaus.jyson import JysonCodec as json
ImportError: No module named xhaus
socket.error: (104, 'Software caused connection abort')
@ypid
Copy link
Contributor Author

ypid commented Aug 21, 2014

Since it is such a pain to get this all working in jython I guess it would be much better to do all the evaluation stuff server side in nodejs. Any thoughts? Would you like to test it?

Doing this on a server would also have the advantage of not adding the dependencies nodejs and pyopening_hours to qat_script …

@simone-f
Copy link
Owner

I guess it would be much better to do all the evalution stuff server side in nodejs.
Doing this on a server would also have the advantage of not adding the dependencies nodejs and pyopening_hours to qat_script …

Yes, it would be better.

Any thoughts? Would you like to test it?

I don't know nodejs but I would willingly test your server.
You could answer to users' requests with a JSON or GPX file (in the second case you colud also test locally the files generated by your server before adding your tool to qat_script).

@ypid
Copy link
Contributor Author

ypid commented Aug 25, 2014

It works great so far. Thanks very much for this plugin.

Are line breaks not supported in the "user_message"?

Could you please test it. I am not sure how to implement support for the other opening_hours related tags in an elegant way. And how can I show different markers for error and warnings.

I would appropriate it (if you have the time) to look over the current pull request and maybe rewrite it a bit if necessary. You know the code base much better than I do.

The other tags which can be covered easily are listed here. The first item in the array is the oh_mode that needs to be fed into new opening_hours().

@ypid
Copy link
Contributor Author

ypid commented Aug 25, 2014

I have to say. Fixing (opening_hours) problems with qat_script is a bliss 👍 The only thing to remember is to check the value if you do not know exactly how it is interpreted because you do not see this in the tool. I usually copy complicated values over to the evaluation tool or use the link generated in the help for the problem 😄 and fix them there because it is easier to see the difference. Also, the evaluation tool will always prettify the value. JOSM does this only if there is a warning for the value (You can force JOSM to generate a warning if you want the prettified value by adding a small mistake like using a dot as hour minute separator in a time like this "12.00-14:00"). The evaluation tool makes it also easier to remove redundant parts of the value.

@simone-f
Copy link
Owner

I'm happy it works :-)

Actualy, there is a problem if you try to download and parse errorOnly or warningOnly, but you can fix it with the following changes (here):

logging.debug('Appending')
errorData = (osmId, (lat, lon), bbox, errorID, user_message,
    {
        'lat': lat,
        'lon': lon,
        'oh_value': oh_value,
        'oh_mode': self.oh_mode
    })
if 'error' in parseTask.errors:
    parseTask.errors['error'].append(errorData)
if errorID in parseTask.errors:
    parseTask.errors[errorID].append(errorData)

Are line breaks not supported in the "user_message"?

You can use <br> since that text is parsed as HTML code. Unfortunately links do not work or you could have added a link to evaluation tool for the error expression.

And how can I show different markers for error and warnings.

Do you mean in the first check with both errors and warnings? Unfortunately a check can have only one icon.

I would appropriate (if you have the time) to look over the current pull request and maybe rewrite it a bit if necessary. You know the code base much better as I do.

Sure. I will refine it and see how to add support for the other tags like "lit" (the next weeks).
I will also disable logging import and substitute urllib/urllib2 with java libraries, because they increase the loading time of the script.

Before I merge the pull request I suggest you to:

  • change the size of icons to 12x12 px or 16x16 px and also change their text from "Err" to "E", so that the icons fit in the GUI (see the list of checks in qat_script dialog). You may also create different icons for the markers by removing the letters, since they cover the OSM data (e.g. a node) affected by the error.
  • Add an icon for your tool. It will be dispayed on the menu and in the dialog tools' combobox
  • Remove makefile

ypid added a commit to opening-hours/opening_hours.js that referenced this pull request Aug 28, 2014
* Added tool icon. See ypid/opening_hours.js@64dd4fe
* Applied patch from @simone-f.
* Using HTML tags for newline in user message.
* Resized and redesigned markers.

Other improvements.
* Removed proof_of_concept.
@ypid
Copy link
Contributor Author

ypid commented Aug 28, 2014

Done ;)

I am looking forward to your cleanup and the integration of the missing tags so that people can actually use it.

@simone-f
Copy link
Owner

simone-f commented Sep 6, 2014

Thanks for the changes and for the fixes in the first commits of your pull request.

simone-f added a commit that referenced this pull request Sep 6, 2014
@simone-f simone-f merged commit f221d9b into simone-f:development Sep 6, 2014
ypid added a commit to opening-hours/opening_hours_server.js that referenced this pull request Sep 6, 2014
@ypid
Copy link
Contributor Author

ypid commented Sep 6, 2014

Thanks for merging and for adopting it.

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

Successfully merging this pull request may close these issues.

None yet

2 participants