search

KWARC / rust-libxml

Rust wrapper for libxml2
https://crates.io/crates/libxml
MIT License
76 stars 38 forks source link

Safe API refactor #34

Closed dginev closed 6 years ago

dginev commented 6 years ago

This PR includes #30 , as initially built by @triptec .

The main difference is that the current PR uses a branch in the main repository, where I can also contribute changes. I have explained the initial changes added here in the comments of #30

dginev commented 6 years ago

I can now say I have tried and am quite happy with the basic ergonomics here:

Some advanced cases remain to be guarded against, somewhat up to our discretion. One example is calling an unbind twice on a node for example, which we could (should?) guard against in Rust territory. A stab of doing that is at 55cb5d5 .

Will wait for @triptec to counter-review and share thoughts, but I think this is a great foundation, and we can keep fleshing out until we have a release candidate. As mentioned my idea for next steps is to create dedicated branches on a couple of my libxml-using projects and seeing how refactoring to this new API feels / and whether it leaks memory.

triptec commented 6 years ago

I think it looks good, only thing I think is missing is before dropping a node I think we have to traverse it's children and remove them from our nodes if they exist there as xmlFreeNode will free those. But I don't think that should block as the work done on this branch makes a lot of things better.

dginev commented 6 years ago

Cool, can merge today/tomorrow then. I also just remembered that I'd have to update CHANGELOG.md before merging, and even the readme, to avoid confusing people -- since we'll land breaking changes to any existing use of the crate.

dginev commented 6 years ago

Ok, done and done. Merging here and @triptec feel free to open new PRs to continue fleshing out the approach. I haven't yet done the third-party repository tests yet, so I may add another patch-pr later this week.