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

[Contributing] [Standards] Added entry for identical comparison #5403

Closed
wants to merge 6 commits into from

Conversation

phansys
Copy link
Contributor

@phansys phansys commented Jun 15, 2015

Q A
Doc fix? yes
New docs? no
Applies to 2.8+
Fixed tickets

| Q             | A
| ------------- | ---
| Doc fix?      | yes
| New docs?     | no
| Applies to    | 2.8+
| Fixed tickets |
Comparisons
-----------

Use `identical comparison`_ when the expected value must match a specific type:
Copy link
Member

Choose a reason for hiding this comment

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

Actually, strict comparison should be used everywhere except when you really need type juggling (in most cases, you don't want it)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

IMO, the most common juggling is when null or an empty string evaluated to false.

@stof
Copy link
Member

stof commented Jun 15, 2015

I think this should be better placed in the coding standards page.

the conventions page is mostly about naming conventions

@phansys phansys force-pushed the conventions_strict_comparison branch from 036261c to fc350ca Compare June 15, 2015 16:08
@phansys
Copy link
Contributor Author

phansys commented Jun 15, 2015

@stof, done.

@phansys phansys changed the title [Contributing] [Conventions] Added entry for identical comparison [Contributing] [Standards] Added entry for identical comparison Jun 15, 2015
Comparisons
-----------

Use `identical comparison`_ when the expected value must match a specific type:
Copy link
Member

Choose a reason for hiding this comment

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

I would go further and recommend to always use the identical comparison unless you have a strong reason to not do that. @wouterj @xabbuh what do you think?

Copy link
Member

Choose a reason for hiding this comment

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

I asked the same previously (which disappeared when moving the note to a different page)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the original note from @stof about identical comparison.

Copy link
Member

Choose a reason for hiding this comment

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

I agree, should be something like "Always use strict comparison unless you need type juggling".

@phansys
Copy link
Contributor Author

phansys commented Jun 16, 2015

Updated message to enforce always to use strict comparison.


if (1 === $integerExpected) {
// ...
}
Copy link
Member

Choose a reason for hiding this comment

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

What about using one code block for this to make it more compact?

Always use `identical comparison`_ unless you need type juggling::

    // use
    if (1 === $integerExcepted) {
        // ...
    }

    // instead of
    if (1 == $integerExcepted) {
        // ...
    }

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Seems good to me @xabbuh.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated.

@phansys phansys force-pushed the conventions_strict_comparison branch from 2f4d218 to b0a441f Compare June 16, 2015 20:37

Always use `identical comparison`_ unless you need type juggling:

.. code-block:: php
Copy link
Member

Choose a reason for hiding this comment

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

you can omit this if you use a double colon at the end of the line before

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I saw that syntax before, but what about specific code highlighting for PHP?

Copy link
Member

Choose a reason for hiding this comment

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

PHP is the default syntax highlighting. So it's used in that case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated.

@xabbuh
Copy link
Member

xabbuh commented Jun 17, 2015

👍

Comparisons
-----------

Always use `identical comparison`_ unless you need type juggling::
Copy link
Member

Choose a reason for hiding this comment

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

I think we should put this in the list of standards as well, instead of a custom section

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You mean in structure section?

Copy link
Member

Choose a reason for hiding this comment

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

yes (sorry if I was a bit too vague)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No problem at all, I'll try to update this PR and #5402 ASAP based in your suggestions.
Thank you.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@wouterj, updated.

@wouterj
Copy link
Member

wouterj commented Jun 26, 2015

+1 from my side (not 100% sure if "Always use ... unless ..." is better or worse than "Unless ..., always use ...", but I'll let our native speakers decide on that one 😉 )

@xabbuh
Copy link
Member

xabbuh commented Jun 28, 2015

Thank you Javier.

xabbuh added a commit that referenced this pull request Jun 28, 2015
…arison (phansys)

This PR was submitted for the 2.8 branch but it was merged into the 2.3 branch instead (closes #5403).

Discussion
----------

[Contributing] [Standards] Added entry for identical comparison

| Q             | A
| ------------- | ---
| Doc fix?      | yes
| New docs?     | no
| Applies to    | 2.8+
| Fixed tickets |

Commits
-------

bacd22e [Contributing] [Standards] Added entry for identical comparison
@xabbuh
Copy link
Member

xabbuh commented Jun 28, 2015

Ryan can always tweak the text later if he disagrees. :)

@xabbuh xabbuh closed this Jun 28, 2015
@phansys phansys deleted the conventions_strict_comparison branch June 29, 2015 18:42
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.

5 participants