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

Fixing value type root element comparison #9

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

Fixing value type root element comparison #9

wants to merge 4 commits into from

Conversation

Starkie
Copy link
Contributor

@Starkie Starkie commented Aug 7, 2018

Solving #8.

Added the safe navigation operator to the affected instructions by the bug, so the exception isn't thrown anymore.

Also, added a test to check if the implementation was correct.

@pwelter34
Copy link
Member

@Starkie If you can fix the build and resolve conflicts, I can get this merged.

@Starkie
Copy link
Contributor Author

Starkie commented Aug 21, 2018

@pwelter34 Thanks for the headsup. I had notes about it at work, and won't be able to check it out until I get back.

But, if I remember correctly, the bug was related on how the pathStack is handled in EntityComparer.cs. The problem starts with the CompareObject() method, during the member analysis:

  1. There is a _pathStack.Push() before the CompareType(), that affects later on.
  2. When, for example, comparing a List by equality, in CompareByEquality(), the full path is obtained for currentPath.
  3. This currentPath is then added an index and assigned to p, which holds the full path for an item of the list.
  4. p is then pushed to the _pathStack.
  5. In the case where the item of the list is another list, when using the CurrentPath() method again, the path will be duplicated. It concatenates all the previous elements with the last one, which was already the full path.

However, this couldn't be solved by simply obtaining the last element of the stack, because this behaviour changes with different combinations objects or nesting.

Before making any kind of drastic change, do you have suggestions on how could this be solved?

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