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 new method IsEquivalent for Element type #91

Closed

Conversation

adolsalamanca
Copy link

@adolsalamanca adolsalamanca commented Jul 29, 2020

New method IsEquivalent for Element type.

Description

As detailed in the Issue #90 , we have worked in a method to perform a DeepEquals between two elements that has been useful for our team, so we wanted to share it with all the users of this fantastic go module.

Details

  • IsEquivalent is a recursive method that enters as deep as possible over all the etree.Element of our etree.Document that represents a XML file.
  • Once there are no more levels, it analyzes the attributes and text inside the XML nodes till the end. If any of those levels is not exactly the same for e and e2, the method returns false.
  • Otherwise, the method returns true.

Tests

  • New TestElementIsEquivalent unit test implemented with a couple of cases.

Additional info

  • We have disccused about the possibility to have options like Whitespace ignoring and Attributes order, however according to the XML spec https://www.w3.org/TR/REC-xml/ neither the attribute order nor the whitespaces are something to worry about so I did not applied it.
  • However, the developer that uses this go mod may want to take any of this things into account, in that case we could implement another method or refine this one, I'm open to both options.

Thanks,

Adolfo Rodriguez

@beevik
Copy link
Owner

beevik commented Aug 4, 2020

Thanks for submitting this pull request!

After spending a few hours with it and trying to make it fit into the existing package, I've concluded that there are too many edge cases the code doesn't handle. Some of the edge cases are:

  1. The function ignores XML comments and directives.
  2. Attributes may appear in any order, but child elements must appear in the same order. This seems inconsistent and may surprise the package's user.
  3. The elements being compared must have their whitespace formatted identically for the function to return true.

It seems strange for the function to care about whitespace formatting but not mismatched XML comments or element order.

While the submitted implementation is fine for some (maybe even most) cases, it is not really suitable for something that claims to be a general-purpose "element equivalence" test, the results of which will confuse many users expecting the edge cases above to be satisfied. In fact, the more I think about it, the more I think the way XML equivalence-checking shoud work is highly application-dependent

For now I am going to recommend that you perform equivalence checking in your own code -- outside of the etree package -- with sometihng like the following:

func Compare(e1, e2 *Element) bool {
	if (e1 == nil) != (e2 == nil) {
		return false
	}
	if (e1 == nil) && (e2 == nil) {
		return true
	}

	children1 := e1.ChildElements()
	children2 := e2.ChildElements()
	if len(children1) != len(children2) {
		return false
	}

	for i, c1 := range children1 {
		c2 := children2[i]
		if c1.Text() != c2.Text() {
			return false
		}
		if len(c1.Attr) != len(c2.Attr) {
			return false
		}
		for _, a := range c1.Attr {
			a2 := c2.SelectAttr(a.Key)
			if a2 == nil || a2.Value != a.Value {
				return false
			}
		}
		if !Compare(c1, c2) {
			return false
		}
	}

	return true
}

I am still open to ideas that could make XML comparison more general purpose. But I think it is not an easy problem to solve (with good performance characteristics).

@adolsalamanca
Copy link
Author

Hi Brett,

Thank you very much again for this great go module and your patience reviewing the proposal, we will continue with our code function to perform comparison for now.

I understand your concerns, and I agree that it could be confusing for users as you mentioned.
I will re-evaluate your proposal and will give a try with those improvements so I could contribute to this fantastic go module.

Thanks!!
Best regards,

Adolfo

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.

2 participants