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

Implement XML configuration loader #90

Merged
merged 1 commit into from
Jun 23, 2018
Merged

Conversation

lucko
Copy link
Contributor

@lucko lucko commented Jan 30, 2018

Implements #57

@lucko lucko changed the title WIP: Implement XML configuration loader Implement XML configuration loader Jan 31, 2018
@lucko
Copy link
Contributor Author

lucko commented Jan 31, 2018

This is no longer WIP.

I'd welcome any feedback / comments.

@FerusGrim
Copy link
Contributor

I had given up hope. I'm in love with you @lucko <3

@lucko
Copy link
Contributor Author

lucko commented Apr 27, 2018

Bump - any update on the status of this?

@kashike
Copy link
Contributor

kashike commented Apr 27, 2018

I'd want to see this merged after #92.

@parlough
Copy link
Contributor

Can you update this with the other changes, as well as style changes you previously made?

@lucko lucko force-pushed the xml branch 2 times, most recently from 7285085 to cf4220a Compare April 29, 2018 12:56
@lucko
Copy link
Contributor Author

lucko commented Apr 29, 2018

I've rebased this against master and updated the style to match the other changes.

I've also fixed an issue with the way lists were serialised, and added a few more tests. Should be good to be reviewed/merged now. :)

@lucko lucko force-pushed the xml branch 2 times, most recently from 0d30d11 to a102ad4 Compare April 29, 2018 13:48
@parlough parlough self-requested a review May 8, 2018 08:39
@parlough
Copy link
Contributor

parlough commented May 8, 2018

I will give this a look soon, but I'd also like to see @zml2008's feedback as this is a fairly large change, adding a completely new loader.

@XakepSDK
Copy link

XakepSDK commented May 10, 2018

map/list types are made wrong, they re must be namespaced and must have schema.

Every xml file(since xml 1.1) must start with xml header.
https://www.w3.org/TR/2006/REC-xml11-20060816/#sec-prolog-dtd
Also, attributes may be not just strings, but enums or sets of tokenized types.
https://www.w3.org/TR/xml11/#attdecls

Also, you don't handle XML comments system.
https://github.com/SpongePowered/configurate/pull/90/files#diff-121eb04ce2db1e49a56ba506f61ebd66R221
This is wrong, XML doesn't allow comments with # or //, they are must be in <!-- COMMENT HERE -->
Also, XML restricts usage of -- inside a comment.
https://www.w3.org/TR/2006/REC-xml11-20060816/#sec-comments

https://www.w3.org/XML/Core/#Publications

@lucko
Copy link
Contributor Author

lucko commented May 10, 2018

Thanks for your feedback.

map/list types are made wrong, they re must be namespaced and must have schema.

Could you elaborate a bit more on exactly what you mean by this? I don't really understand what you're talking about.

Every xml file(since xml 1.1) must start with xml header.

The loader already had the option to include the header, I've now set it to be enabled by default. Thanks :)

Also, attributes may be not just strings, but enums or sets of tokenized types.

The page you linked doesn't make much sense to me. The native Java library being used for parsing doesn't seem to support anything other than String-String pairs for attributes.

https://docs.oracle.com/javase/7/docs/api/org/w3c/dom/Attr.html

Also, you don't handle XML comments system.

I've implemented an appropriate comment handler now and added a test.

@XakepSDK
Copy link

XakepSDK commented May 12, 2018

  1. Ok, just skip it
  2. Does your comments impl. allow me to write multi-line comments like this:
<!--
My multiline
comment here
-->

If no, impl is wrong.

@lucko
Copy link
Contributor Author

lucko commented May 12, 2018

No, the comment handler forms comments like this

<!--
 ~ My multiline
 ~ comment here
 -->

It will still correctly parse your example

@FerusGrim
Copy link
Contributor

@lucko Still looking forward to this being merged! Are there any concerns besides the issues brought forth by @XakepSDK ?

Additionally, it looks as if the project has been switched over to Gradle, causing some issues as this PR was started when it was still using Maven. <3

@lucko
Copy link
Contributor Author

lucko commented Jun 21, 2018

Yea, will need to be rebased.

I'm not sure what the status is, I would prefer to gather more feedback / approval from people before this is merged though.

@lucko lucko force-pushed the xml branch 2 times, most recently from a494aba to 7e32f5f Compare June 21, 2018 21:37
@lucko
Copy link
Contributor Author

lucko commented Jun 21, 2018

It's rebased now, so can be merged when ready.

I've also added a few more tests.

@bloodmc
Copy link

bloodmc commented Jun 23, 2018

@Meronat Please review so we can get this merged.

@kashike kashike self-assigned this Jun 23, 2018
@kashike
Copy link
Contributor

kashike commented Jun 23, 2018

Thanks, @lucko.

@kashike kashike merged commit ca6b103 into SpongePowered:master Jun 23, 2018
@lucko lucko deleted the xml branch June 23, 2018 07:14
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.

6 participants