Skip to content

Conversation

@rogervila
Copy link
Owner

@rogervila rogervila commented Nov 30, 2020

@rogervila rogervila self-assigned this Nov 30, 2020
@rogervila rogervila marked this pull request as ready for review December 2, 2020 12:28
}

if ($value1 != $value2) {
if ($value1 !== $value2) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it necessary to compare types too?
What is an expected result for comparing these two arrays: ['a' => 1] and ['a' => '1']
What if the compare method has the 3rd parameter $strict = true?

Copy link
Owner Author

Choose a reason for hiding this comment

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

Hi,

The example you exposed could be a bug detection in a codebase where an integer is converted by error to a string, or vice-versa.

IMO strict comparisons should be the only comparison type to use since 1 is totally different to '1'

Anyway, if using a loose comparison method is a required feature, I will not remove it.

I agree with adding a 3rd parameter, but since version 2 is a great opportunity for breaking changes, it will be true by default.

I will also add specific methods like strictCompare() and looselyCompare() since they look clearer for me.

Thank you very much for your suggestion.

@rogervila rogervila added the WIP label Dec 3, 2020
@sonarqubecloud
Copy link

sonarqubecloud bot commented Dec 3, 2020

Kudos, SonarCloud Quality Gate passed!

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

No Coverage information No Coverage information
0.0% 0.0% Duplication

@rogervila rogervila removed the WIP label Dec 3, 2020
@rogervila rogervila merged commit 318effb into master Dec 3, 2020
@rogervila rogervila deleted the version2 branch December 3, 2020 14:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants