-
Notifications
You must be signed in to change notification settings - Fork 564
[Xamarin.Android.Build.Tasks] Fix Removal of Non Duplicate elements #856
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
Fixes https://bugzilla.xamarin.com/show_bug.cgi?id=59473 Commit d079a42 went a bit too far when removing duplicates. It removed ANY duplicate entry that appeared anywhere in the document. What we really wanted to remove full duplicates that exist at the same level as the current element in the document. For example ```xml <foo> <bar name="bar1"> <bar2 /> </bar> <bar name="bar2"> <bar2 /> </bar> <dupe/> <dupe/> </foo> ``` `bar2` should NOT be removed but one of the `dupe` values should. So this commit reworks the RemoveDuplicates code to handle the correct logic. It also adds a unit test for this exact senario.
- Loading branch information
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -402,11 +402,18 @@ void MergeLibraryManifest (string mergedManifest) | |
| } | ||
| } | ||
|
|
||
| public IEnumerable<XElement> ResolveDuplicates (IEnumerable<XElement> elements) | ||
| { | ||
| foreach (var e in elements) | ||
| foreach (var d in ResolveDuplicates (e.Elements ())) | ||
| yield return d; | ||
| foreach (var d in elements.GroupBy (x => x.ToFullString ()).SelectMany (x => x.Skip (1))) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't understand how this method works. :-( This part appears to be identical to what's being removed, i.e. this fragment is "buggy" (hence it needing to be fixed in the first place). Presumably then, the above Assuming that the How's this work? It's blowing my mind. :-(
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The previous method used <foo>
<bar>
<bug/>
</bar>
<dar>
<bug/>
</dar>
<too/>
<too/>
</foo>In the previous method both |
||
| yield return d; | ||
| } | ||
|
|
||
| void RemoveDuplicateElements () | ||
| { | ||
| var duplicates = doc.Descendants () | ||
| .GroupBy (x => x.ToFullString ()) | ||
| .SelectMany (x => x.Skip (1)); | ||
| var duplicates = ResolveDuplicates (doc.Elements ()); | ||
| foreach (var duplicate in duplicates) | ||
| duplicate.Remove (); | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Aside: these lines full of whitespace are weird.