-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
JsonNull return false when comparing against null #842
JsonNull return false when comparing against null #842
Conversation
I'ved modified JsonNulls comparison methods to return false when compared against null, the matching test have also been updated in change Fix dotnet#820
I do not believe the current tests which are failing are related to the changes I made as it network timeouts. I also have a couple of questions as this is my first PR: As it's just a timeout, should the failed tests be rerun? |
Yes. However, what I tend to do (and encourage) is that I do a cursory search of the test failure/exception in the repo to see if an issue already exists tracking the intermittent failure, and if not, then file one, so that it gets tracked and resolved. Here are some examples: https://github.com/dotnet/corefx/issues/42583, https://github.com/dotnet/corefx/issues/42077, https://github.com/dotnet/corefx/issues/41881
Generally, you'd tag the area owners to help review the PR: I am happy to look it over and provide feedback. |
src/libraries/System.Text.Json/src/System/Text/Json/Node/JsonNull.cs
Outdated
Show resolved
Hide resolved
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.
Other than updating xml doc comments, looks good.
Right, the test failure in CI is unrelated: https://github.com/dotnet/corefx/issues/20069#issuecomment-566292587 |
I have updated the xml comments to reflect the changes that was made in JsonNull and removed left over comment in JsonNullTests
I have updated the XML docs and this should be ready for a review The test failure looked unrelated to me and I've commented on #702 |
src/libraries/System.Text.Json/src/System/Text/Json/Node/JsonNull.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Text.Json/src/System/Text/Json/Node/JsonNull.cs
Outdated
Show resolved
Hide resolved
Applied the suggested change to correct xml documentation Co-Authored-By: Ahson Khan <[email protected]>
Applied the suggested change to correct xml documentation Co-Authored-By: Ahson Khan <[email protected]>
Thanks for fixing this @henrikse55 - let me know if you are interested in picking up other issues :) https://github.com/dotnet/runtime/issues?q=is%3Aopen+is%3Aissue+label%3Aarea-System.Text.Json+label%3Aup-for-grabs |
I'ved modified JsonNulls comparison methods to return false when
compared against null, the matching test have also been updated
in change
Fix #820