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

Use string formatting for XML output #5

Closed
ghost opened this issue Jul 16, 2016 · 4 comments · Fixed by #20
Closed

Use string formatting for XML output #5

ghost opened this issue Jul 16, 2016 · 4 comments · Fixed by #20

Comments

@ghost
Copy link

ghost commented Jul 16, 2016

Currently the BCE uses raw string output to create XML elements:

self.writel(S_FX, 3, '<newparam sid="' + surface_sid + '">')

This is both hard to read and hard to write. The Blender team suggests using string formatting like they did here:

fw('%s<Coordinate USE=%s />\n' % (indent, mesh_id_coords))

I suggest something entirely different and in my opinion easier to both read and write (since it takes care of indentation automatically, and is also less error-prone) - using xml.etree.cElementTree to generate the XML (which is fast enough) and then use xml.dom.minidom to prettify/indent it (both are included in the standard library).

@set-killer
Copy link
Contributor

This is related to about 30-40% of the code.

Which of the both methods is better to use? In my opinion using xml.etree.cElementTree would not make the code more easy to read and it will be harder to make the parenting of the XML nodes.
Anyone with experience on this?

(Just discussing the issue...)

@ghost
Copy link
Author

ghost commented Jul 19, 2016

@set-killer I think that if unit tests for the XML generation are written, the format method can be just as reliable (and faster).

@ghost ghost mentioned this issue Aug 15, 2016
@ghost
Copy link
Author

ghost commented Aug 15, 2016

I think it's best if at least for the time being, since it's what the Blender dev requested and it'd be great to have Blender devs working on this plugin as soon as possible, we go ahead with format for the XML output.

@set-killer If you can take this and push a commit in a reasonable time please tell me, otherwise I can do it myself in a few days (hopefully).

@ghost ghost changed the title Replace raw XML output with builder Use string formatting for XML output Aug 15, 2016
@set-killer
Copy link
Contributor

Yes, working on it.

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.

2 participants