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

Generate valid XML files #842

Open
excusethepaywall opened this issue Jan 2, 2020 · 4 comments · May be fixed by #1036
Open

Generate valid XML files #842

excusethepaywall opened this issue Jan 2, 2020 · 4 comments · May be fixed by #1036

Comments

@excusethepaywall
Copy link

The history.xml and anime.xml files generated by Taiga have multiple roots ("meta" plus another file-specific tag), which does not conform to the XML standard and makes these files incompatible with any parser that conforms to the XML standard, like python's xml module.

Please consider reorganizing these xml files to conform with the XML standard. Thank you.

@erengy
Copy link
Owner

erengy commented Jan 7, 2020

I always assumed the document itself was the (unnamed) root. Makes more sense to me, but apparently that is not the case. Thanks!

I'm actually considering to switch to an entirely different format in future releases. If not, I'll make sure to reorganize and validate the files.

@Bu11etmagnet
Copy link

Note the last bullet point in "key syntax rules"https://en.wikipedia.org/wiki/Well-formed_document

There is a single "root" element that contains all the other elements.

Also

When I tried to open anime.xml with Notepad++ (with the XML tools plugin), I got the following popup:

---------------------------
XML Tools plugin
---------------------------
XML Parsing error at line 5: 

Extra content at the end of the document

---------------------------
OK   
---------------------------

One advantage of using XML is that it has very strict rules when it comes to parsing. This makes it easy to detect errors.

I see you're using pugixml to parse XML files. I see pugixml accepts an input like this:

<meta>
	<version>1.3.1</version>
</meta>
<database>
</database>

without returning an error. This means pugixml is a non-conforming parser, because

An XML processor that encounters a violation of the well-formedness rules is required to report such errors and to cease normal processing.

(from the same Wikipedia page; emphasis mine).

So pugixml is not an XML parser. It parses something which is almost, but not quite, entirely unlike XML. I filed zeux/pugixml#337

Luckily, changing the format of history.xml and anime.xml should not be an onerous task (says the guy who doesn't have to do it). If the <meta> element can only contain the version information, that could be moved into an attribute of the <history> or <library> element:

<history version="1.3.1">
etc.
</history>

@erengy erengy changed the title Generated xml files have multiple roots Generate valid XML files May 27, 2020
@FreezyLemon
Copy link

FreezyLemon commented Oct 10, 2021

I'd like to go for this one. The one topic I'm not very sure about is compatibility.
My idea would be simple: Change from

<?xml version="1.0"?>
<meta>
  <version>1.3.1</version>
</meta>
<library>
...

to

<?xml version="1.0"?>
<library version="1.3.1">
...

Should it maybe be called 'taiga-version' or something? (We have an XML version attribute right above it)

And about compat: There's some places like

auto version = ReadXmlVersion(document);
HandleCompatibility(version);

If I change the code to use attributes instead of meta elements, the version that is currently inside the XML files will be ignored (version = 0.0.0). Should the old Read procedure be left in the code for compatibility? Or does this not matter, since there is nothing (except for the version itself) that changed in the XML?

@FreezyLemon
Copy link

FreezyLemon commented Oct 10, 2021

Ok, after reading through the compat code it seems pretty clear.
anime.xml and history.xml do not have downwards-compatibility code since 1.4.0-beta1 (while 1.3.1 still has some code for XML files from versions <1.3.0). This means that even if the version is set to 0.0.0, nothing happens (since there is no compat code).

Now, there is only settings.xml left. The file currently looks like this:

<?xml version="1.0"?>
<settings>
  <meta version="1.3.1" />
  <account>
...

This is different to the other meta elements, and does not violate the 'one root element' rule in its current state. It would be cleaner to change this to

<?xml version="1.0"?>
<settings version="1.3.1">
  <account>
...

BUT there is still compat code for settings.xml. Keeping compatibility for both 'styles' of settings.xml shouldn't be hard, but this change isn't really necessary (as it is already valid XML, even if it is not uniform to the other files). I'd argue for keeping it as-is

@FreezyLemon FreezyLemon linked a pull request Oct 10, 2021 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants