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

SE: Learn bool constraints from Equals with bool parameters. #7899

Merged
merged 10 commits into from
Aug 28, 2023

Conversation

mary-georgiou-sonarsource
Copy link
Contributor

related to #7704

@mary-georgiou-sonarsource mary-georgiou-sonarsource added the Sprint: SE Short-lived* label for epic MMF-3077 *troll label Aug 25, 2023
@mary-georgiou-sonarsource mary-georgiou-sonarsource moved this from In progress to Review in progress in Best Kanban Aug 25, 2023
Copy link
Contributor

@Tim-Pohlmann Tim-Pohlmann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's simplify this commit. Also add tests for the following:

  • bool vs object
  • bool vs null
  • bool vs bool?
  • bool? vs bool?
  • bool? vs object
  • bool? vs null

@github-actions github-actions bot moved this from Review in progress to In progress in Best Kanban Aug 25, 2023
@github-actions github-actions bot moved this from In progress to Review in progress in Best Kanban Aug 28, 2023
Copy link
Contributor

@Tim-Pohlmann Tim-Pohlmann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Round 2

Comment on lines +970 to +974
[DataTestMethod]
[DataRow("true", "null", false)]
[DataRow("null", "true", false)]
[DataRow("null", "(bool?)true", false)]
public void Invocation_ObjectEqualsMixedNullObjectArguments_LearnResult(string left, string right, bool expectedResult)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you can just incorporate this into Invocation_BoolEquals_LearnsResult.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It calls ObjectEquals and tests this method's behavior in practice - but it's not easy to incorporate into ObjectEquals either :/

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe just add it to Invocation_ObjectEquals_LearnResult then.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not straightforward - as discussed will leave as is.

@github-actions github-actions bot moved this from Review in progress to In progress in Best Kanban Aug 28, 2023
@sonarcloud
Copy link

sonarcloud bot commented Aug 28, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@sonarcloud
Copy link

sonarcloud bot commented Aug 28, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

100.0% 100.0% Coverage
0.0% 0.0% Duplication

@github-actions github-actions bot moved this from In progress to Review in progress in Best Kanban Aug 28, 2023
Copy link
Contributor

@Tim-Pohlmann Tim-Pohlmann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Take a last look at the unresolved conversation to see if it makes sense.

@github-actions github-actions bot moved this from Review in progress to Review approved in Best Kanban Aug 28, 2023
@mary-georgiou-sonarsource mary-georgiou-sonarsource merged commit 84176ea into master Aug 28, 2023
27 checks passed
Best Kanban automation moved this from Review approved to Validate Peach Aug 28, 2023
@mary-georgiou-sonarsource mary-georgiou-sonarsource deleted the mary/equals-constraints branch August 28, 2023 13:19
@Tim-Pohlmann Tim-Pohlmann moved this from Validate Peach to Done in Best Kanban Aug 31, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Sprint: SE Short-lived* label for epic MMF-3077 *troll
Projects
Best Kanban
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

2 participants