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

Port from xml-rs to quick-xml #88

Merged
merged 2 commits into from
Nov 18, 2022
Merged

Conversation

jcgruenhage
Copy link
Contributor

@jcgruenhage jcgruenhage commented Nov 15, 2022

Follow-up from #78. Things that happened since:

  • rebased this on the current changes on master
  • dropped the quick-xml impl into where the xml-rs one used to be, instead of offering quick-xml as a feature
  • fixed up the issue with pending collections
  • fixed a whole lot of clippy issues while I was at it

Thanks to @pr2502 for doing the heavy lifting here, most stuff was already in pretty good shape so I mostly just moved stuff around and did a few little touch-ups here and there.

@jcgruenhage jcgruenhage mentioned this pull request Nov 15, 2022
Copy link

@RickyDaMa RickyDaMa left a comment

Choose a reason for hiding this comment

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

Hi there!

While not a maintainer of this repo, I am a keen Rustacean with an interest in the original issue, so I hope you don't mind my feedback.

Thanks for continuing on this work!

rustfmt.toml Outdated Show resolved Hide resolved
src/dictionary.rs Outdated Show resolved Hide resolved
src/error.rs Outdated Show resolved Hide resolved
src/serde_tests.rs Show resolved Hide resolved
src/stream/binary_reader.rs Outdated Show resolved Hide resolved
src/stream/xml_reader.rs Outdated Show resolved Hide resolved
src/stream/xml_reader.rs Outdated Show resolved Hide resolved
src/serde_tests.rs Outdated Show resolved Hide resolved
@jcgruenhage jcgruenhage force-pushed the quick-xml branch 2 times, most recently from 254d6df to 39ac42a Compare November 16, 2022 11:21
@jcgruenhage
Copy link
Contributor Author

Thanks for your feedback, @RickyDaMa. I've applied most of your suggestions, they do improve the code quite a bit. I'll be honest: I haven't completely read the code that I committed, just the bits I needed to touch to apply the feedback from over at #78 😅, as well as a bit required for #52 later on, although I haven't started implementation on that one yet, as I'd want this here and #85 to be merged first.

@RickyDaMa
Copy link

No problem, I will admit I originally did the review thinking this was a repository I had some association with, but glad I helped nonetheless 😅 Hope this can get merged!

@ebarnard
Copy link
Owner

@RickyDaMa Thanks for doing the review for me 😁! I'll try and take a look at it later today.

Copy link
Owner

@ebarnard ebarnard left a comment

Choose a reason for hiding this comment

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

It's looking pretty good @jcgruenhage, just a few changes. I've only reviewed the first commit, please could you drop the clippy changes from this PR for the time being and put them in another once this is merged.

src/stream/xml_reader.rs Outdated Show resolved Hide resolved
src/stream/xml_reader.rs Outdated Show resolved Hide resolved
src/date.rs Outdated Show resolved Hide resolved
src/date.rs Outdated Show resolved Hide resolved
src/stream/binary_writer.rs Outdated Show resolved Hide resolved
src/stream/xml_reader.rs Outdated Show resolved Hide resolved
src/stream/xml_reader.rs Outdated Show resolved Hide resolved
src/stream/xml_reader.rs Outdated Show resolved Hide resolved
src/stream/xml_writer.rs Outdated Show resolved Hide resolved
src/stream/xml_writer.rs Show resolved Hide resolved
@jcgruenhage
Copy link
Contributor Author

Thanks for the extensive review @ebarnard, I'll try to get the requested changes in today :)

@jcgruenhage
Copy link
Contributor Author

I've addressed all of the feedback except for the Reader::check_end_names thing so far, because that'll be a slightly bigger change. I hope I'll get to that later as well though.

@jcgruenhage
Copy link
Contributor Author

Okay, @ebarnard, your feedback should now be fully addressed, let me know if there is anything else you'd want changed here.

@pr2502
Copy link
Contributor

pr2502 commented Nov 17, 2022

hi @jcgruenhage thank you for finishing this! i'm happy you decided to tag me as a co-author, but may i ask you to replace my old tag with max <[email protected]>. thanks a lot ^^

@jcgruenhage
Copy link
Contributor Author

Sure thing!

Copy link
Owner

@ebarnard ebarnard left a comment

Choose a reason for hiding this comment

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

Two more small questions and then we're good to go. Thank you so much @jcgruenhage for taking time to work on this.

src/stream/xml_reader.rs Outdated Show resolved Hide resolved
src/stream/xml_writer.rs Show resolved Hide resolved
@jcgruenhage
Copy link
Contributor Author

@ebarnard I think this is ready now. One question: What's going to be the next released version? Asking because of the deprecation warning on XmlWriteOptions::indent_string

@ebarnard
Copy link
Owner

Think it should be 1.4

@jcgruenhage
Copy link
Contributor Author

Then the deprecation is already good as it is. With your approval on it now, do you want to merge it?

@ebarnard ebarnard merged commit 5fdedbe into ebarnard:master Nov 18, 2022
@lucasfernog
Copy link

@ebarnard do you have an idea when this could be released?

@ebarnard
Copy link
Owner

v1.4 has now been released.

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.

5 participants