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

WIP: Validate documents against the openmensa schema #40

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

klemens
Copy link
Collaborator

@klemens klemens commented Oct 19, 2016

This is useful for development, as you may not notice when the generated xml is not valid (as I did in #37).

The first commit adds validating support to parse.py, which makes lxml a mandatory dependency for dev.

I also think it would be useful to add validation to the travis builds, maybe by replacing the wget commands with a python script that validates the returned documents. What do you think?

This makes lxml a mandatory dependency.
@mswart
Copy link
Owner

mswart commented Oct 19, 2016

Looks good as fallback layer. I am unsure whether it is good to have the schema copied into this project. Independent from that I have updated PyOpenMensa (c3fd2ec) to raise ValueErrors on empty category names and empty notes: mswart/pyopenmensa@05d9fd6.

For now I would keep wget on travis. My goal would be to adjust PyOpenMensa to prevent all invalid XML generation.

@klemens
Copy link
Collaborator Author

klemens commented Oct 19, 2016

You are right, adding the schema to the repository isn't so great. I added it, because downloading it on every call to parse.py seemed even worse. However making sure PyOpenMensa produces valid documents is the far better option.

I think validating on travis would still be useful, especially as you are using it to directly deploy on a successful build. Instead of using a python script for validation we could also just pipe the files through xmllint and download the schema once for every build, so we don't have to add it to the repo.

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

Successfully merging this pull request may close these issues.

3 participants