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

Convert from minidom to roxml #22

Open
wants to merge 8 commits into
base: master
Choose a base branch
from
Open

Convert from minidom to roxml #22

wants to merge 8 commits into from

Conversation

AlecTroemel
Copy link
Owner

@AlecTroemel AlecTroemel commented Mar 24, 2023

all tests pass, and generate the same json from test_xml_files examples

closes #21

@Vagelis-Prokopiou
Copy link

Hi @AlecTroemel. I am waiting on this too :-)

@rimutaka
Copy link
Collaborator

There are some parsing differences between these 2 crates. I wonder if it may impact existing implementations.
For example, it handles whitespace differently: https://github.com/RazrFalcon/roxmltree/blob/master/docs/parsing.md#whitespaces

Did we forget to bump the version?

@AlecTroemel
Copy link
Owner Author

@rimutaka hmm.. that could explain why i had to change the Null value config here https://github.com/AlecTroemel/quickxml_to_serde/pull/22/files#diff-a0262fea108af44a98cc49e5eb72641963dd816513f2d0a22eb738fcfed55ebeR353
to get tests passing.

I'll take another look there, ideally we'd be able to pass tests without making any changes to them

@AlecTroemel
Copy link
Owner Author

aha, I think i've fixed the change, tests now pass with no changes from master branch!

@AlecTroemel
Copy link
Owner Author

this MR does make the name of this repo incorrect.. Not sure the best way to handle that 🤔

@Ryan-Knepp
Copy link

I think the version should probably be bumped to 1.0 since the public api is returning roxmltree::Error errors now.

@cjschneider2
Copy link
Contributor

cjschneider2 commented Oct 18, 2023

@AlecTroemel just a note; this branch won't compile with the json_types feature enabled.

I also have two changes in behavior while running this branch across my local test suite (after propagating path to convert_text):

  • non-declared namespaces now error with UnknownNamespace while parsing
  • whitespace before the initial xml tag errors with ParserError on the first < character in this example
        let xml = r#"
            <?xml version="1.0" encoding="UTF-8"?>
            "#;

match convert_node(&child, config, &path) {
Some(val) => {
let name = &child.tag_name().name().to_string();
println!("{:?}", name);
Copy link
Contributor

@cjschneider2 cjschneider2 Oct 19, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this println!() should be removed

@cjschneider2
Copy link
Contributor

another behavioral change I have noticed though I don't see immediately why; is that when using types the it used to come out as @namespace:type but now only generates @type.

@Keats
Copy link

Keats commented May 27, 2024

Any updates on that?

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.

Upgrade minidom to v 0.13
6 participants