-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
AVRO-3503 Fix usage of 'is' and 'as' in Equals #1666
base: main
Are you sure you want to change the base?
Conversation
Currently working on tests. |
I recommend to add the unit tests first into a different PR for each Equals code which will be changed. Once there is test coverage, the Equals code should be modified in a seperate PR. |
I am not sure changing to |
I can break it out into separate PR's. I'm going to use this as a bucket to check against the CI pipelines. As for SequenceEqual, I find it better than the for loops and nested if statements as long as it is comparing simple types. I'm reluctant to use it for complex types. |
The complexity of reviewing the SequenceEqual change is definetely higher than the actual problem the ticket scopes for. E.g. SequenceEqual is using the default comparer The unit tests should provide the baseline for the changes you make, unless the changes are trivial. |
The other issue is changing the code behaviour of the Equals functions. It is standard practice to either throwing a exception if the argument is null or returning false. Some of the changes would return false instead of throwing the exception (which is the current implementation in some of those codes). Which might be ok, if we mark this PR as a major release PR, but in that case, the scope is definetely bigger than it is described in the ticket. |
In general, I'd recommend having A.Equals(object obj) compare |
Jira
Tests
Commits
Documentation