-
Notifications
You must be signed in to change notification settings - Fork 16
CI: Validate token XML files against Cockatrice's XSD schema #93
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
Conversation
|
From the Travis logs: |
|
As per Cockatrice/Cockatrice#2787 and Cockatrice/Cockatrice#3710
If we're automating checks, we really should try to revisit using an XML 1.1 XSD so we don't get complaints about a perfectly valid XML file. |
No, if it runs in Cockatrice, then the file is perfectly valid. The XSD is incomplete and incorrect documentation that defines a subset of the XML schema that will work with Cockatrice. If we merge this, the checks are just going to be ignored because they can't be trusted. Once that happens, you will find it difficult to get that trust back if a more accurate tool is implemented later. |
|
Sure, that makes sense. As long as the checks are not helpful we should not merge this.
So the XSD at Cockatrice needs another update? |
|
I revisited this PR to update it in order to use GitHub Actions over Travis to stay in line with all CI stuff in the Cockatrice organisation. It integrates better with the rest of the GitHub page and don't take the users away when reviewing PR's. It's all right in the Outdated textSee #93 (comment)
Somebody with better XML/XSD knowledge needs to help here. :) This does not change Psi's point that this should not be merged until the XSD is updated and this check can be trusted. Which requires the XSD to be updated to 1.1.
There is xmlschema, a Python library, with XSD 1.1 support! |
|
The last mystical error was due to |
|
Example of the xmllint run introduced here being useful: The CI run would have reported the issue in the introducing PR right away before even merging: Note: |
My understanding is that our cards.xsd files care about the order of elements in an entry, whereas Cockatrice doesn't really care as long as they are present. For example, #96 rearranged two lines to make the validator happy, but Cockatrice can display all that information correctly either way. Based on Psithief's explanation, this seems to be necessary in our case because of the constraints of XSD 1.0. I agree that if this validator flagged element order issues frequently, it would get annoying and soon be ignored since the file is valid in Cockatrice. However, this seems like it would be a pretty rare false positive nowadays as we basically always put elements in the same order. cards.xml is generated automatically (right?), my updates to tokens.xml are generated near enough to automatically (and it seems like other people copy and paste the blank template if they do it manually so order is still preserved), and the challenge tokens will probably never be updated again (besides the proposition to swap them to v4, which was also done automatically). It sounds like it would definitely be advantageous to switch to XSD 1.1 at some point, but, from what I have read in this PR, it doesn't seem like not being able to make that switch should block automatic XML validation. Of course, that is just based on this element ordering issue. Maybe there are other potentially annoying false positives that I am not aware of. |


fixes #65
Travis CI will now automatically check and report if tokens.xml is valid against Cockatrice DB schema v4, and challange-tokens.xml against Cockatrice DB schema v3.
On every change made to the repo or any PR created this will trigger and has results shortly after.
Checks are super fast, but it looks like we are not perfectly valid currently?