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 Yoda conditions #5402

Closed
wants to merge 1 commit 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

@stof
Copy link
Member

stof commented Jun 15, 2015

This is part of the coding standards actually (and I'm quite sure it is already documented)

@phansys
Copy link
Contributor Author

phansys commented Jun 15, 2015

I didn't see any entry about this @stof: https://symfony.com/doc/current/contributing/code/standards.html

@stof
Copy link
Member

stof commented Jun 15, 2015

I thought it was mentioned. Can you update your PR to mention it on this page instead then ?

@phansys
Copy link
Contributor Author

phansys commented Jun 15, 2015

@stof, updated.

@phansys phansys changed the title [Contributing] [Conventions] Added entry for Yoda conditions [Contributing] [Standards] Added entry for Yoda conditions Jun 15, 2015
Use `Yoda condition`_ when you need to check a variable against a value.
The main goal with this syntax is to avoid an accidental assignment inside the
condition statements:

Copy link
Member

Choose a reason for hiding this comment

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

What if we reword this paragraph to make it much shorter?

Use `Yoda conditions`_ when checking a variable against a value to avoid an
accidental assignment inside the condition statements:

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@javiereguiluz, updated.

@soullivaneuh
Copy link
Contributor

@stof @phansys BTW, having a little more explanation of why this choice on the docs would be useful for non adept developers. 👍

@javiereguiluz
Copy link
Member

👍 I think this PR is now ready to be labeled as finished and to be merged soon. Thanks!

@xabbuh
Copy link
Member

xabbuh commented Jun 22, 2015

👍

@soullivaneuh
Copy link
Contributor

Regarding to WordPress coding style from where Yoda condition from (AFAIK):

Yoda conditions for <, >, <= or >= are significantly more difficult to read and are best avoided.

Is I understand well, Yoda condition should be avoided for <, >, <= or >=, isn't it?

If yes, it should be clearly specified on this doc too.

----------

Use `Yoda condition`_ when checking a variable against a value to avoid an
accidental assignment inside the condition statements::
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 add this to the list of standards, instead of creating a custom section purely for one standard.

* Use `Yoda conditions`_ when checking a variable against a value to avoid an
  accidental assignment inside the condition statements.

Copy link
Contributor

Choose a reason for hiding this comment

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

I would also specify:

Yoda condition are not recommended for operators like `<`, `>`, `<=` or `>=`.

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, you mean in structure section?

@soullivaneuh, I think there is no possibility of accidental assignment in < and >, so I agree with both of these.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved inside "structure" section. It would be nice to know what others think about @soullivaneuh proposal.

@@ -100,6 +100,9 @@ Structure

* Place unary operators (``!``, ``--``, ...) adjacent to the affected variable;

* Use `Yoda condition`_ when checking a variable against an expression to avoid
Copy link
Member

Choose a reason for hiding this comment

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

I guess the correct wording would be "Use Yoda conditions" or "Use a Yoda condition" (the first sounds more natural to me).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

| Q             | A
| ------------- | ---
| Doc fix?      | yes
| New docs?     | no
| Applies to    | 2.8+
| Fixed tickets |
@phansys phansys force-pushed the conventions_yoda_conditions branch from d1e9207 to 844ac8e Compare June 26, 2015 15:27
@@ -100,6 +100,9 @@ Structure

* Place unary operators (``!``, ``--``, ...) adjacent to the affected variable;

* Use `Yoda conditions`_ when checking a variable against an expression to avoid
Copy link
Contributor

Choose a reason for hiding this comment

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

Please consider my comment: #5402 (comment)

Is contributors are agree with it of course.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes @soullivaneuh, I'd like to hear more thoughts on this.

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

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

Discussion
----------

[Contributing] [Standards] Added entry for Yoda conditions

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

Commits
-------

023a96f [Contributing] [Conventions] Added entry for Yoda conditions
weaverryan added a commit that referenced this pull request Jun 28, 2015
@weaverryan
Copy link
Member

Thanks @phansys! @soullivaneuh I took some liberty and added your suggestion at sha: d606f3e

Cheers!

@weaverryan weaverryan closed this Jun 28, 2015
@soullivaneuh
Copy link
Contributor

@weaverryan 👍

weaverryan added a commit that referenced this pull request Jun 28, 2015
* 2.3:
  Changed PhpStormOpener to PhpStormProtocol
  [#5402] Being explicit what this applies to (it should not apply to things like >=)
  [Contributing] [Conventions] Added entry for Yoda conditions
  Document security.switch_user event
  Added some more docs about the remember me feature
  Fixed a minor grammar issue
  Fixed a minor grammar issue
  Added support for standard Forwarded header
  Added support for standard Forwarded header
  Fixed issues reported by @xabbuh
  Remove the Propel book chapter and explain why we do that
weaverryan added a commit that referenced this pull request Jun 28, 2015
* 2.6:
  Changed PhpStormOpener to PhpStormProtocol
  [#5402] Being explicit what this applies to (it should not apply to things like >=)
  [Contributing] [Conventions] Added entry for Yoda conditions
  Added the "payload" option back
  Show annotations first
  Reordered the code blocks to show Annotations, YAML, XML and PHP
  Fixed the issues reported by @xabbuh
  Finished the documentation of the new data comparison validators
  Added information about the new date handling in the comparison constraints and Range
  Document security.switch_user event
  Added some more docs about the remember me feature
  Fixed a minor grammar issue
  Fixed a minor grammar issue
  Added support for standard Forwarded header
  Added support for standard Forwarded header
  Fixed issues reported by @xabbuh
  Remove the Propel book chapter and explain why we do that
weaverryan added a commit that referenced this pull request Jun 28, 2015
* 2.7: (21 commits)
  Changed PhpStormOpener to PhpStormProtocol
  [#5402] Being explicit what this applies to (it should not apply to things like >=)
  [Contributing] [Conventions] Added entry for Yoda conditions
  Added the "payload" option back
  Show annotations first
  Reordered the code blocks to show Annotations, YAML, XML and PHP
  Fixed the issues reported by @xabbuh
  Finished the documentation of the new data comparison validators
  Added information about the new date handling in the comparison constraints and Range
  Document security.switch_user event
  [#5332] typo
  [#5335] Minor tweaks
  Added some more docs about the remember me feature
  [Serializer] Updated the cookbook.
  Fixed a minor grammar issue
  Fixed a minor grammar issue
  [Serializer] ObjectNormalizer, object_to_populate doc. Minor enhancements.
  Added support for standard Forwarded header
  Added support for standard Forwarded header
  Fixed issues reported by @xabbuh
  ...
weaverryan added a commit that referenced this pull request Jun 28, 2015
* 2.8: (22 commits)
  Changed PhpStormOpener to PhpStormProtocol
  [#5402] Being explicit what this applies to (it should not apply to things like >=)
  [Contributing] [Conventions] Added entry for Yoda conditions
  Added the "payload" option back
  Show annotations first
  Reordered the code blocks to show Annotations, YAML, XML and PHP
  Fixed the issues reported by @xabbuh
  Finished the documentation of the new data comparison validators
  Added information about the new date handling in the comparison constraints and Range
  Document security.switch_user event
  [#5332] typo
  [#5335] Minor tweaks
  document new Doctrine APC cache service
  Added some more docs about the remember me feature
  [Serializer] Updated the cookbook.
  Fixed a minor grammar issue
  Fixed a minor grammar issue
  [Serializer] ObjectNormalizer, object_to_populate doc. Minor enhancements.
  Added support for standard Forwarded header
  Added support for standard Forwarded header
  ...
@phansys phansys deleted the conventions_yoda_conditions branch June 29, 2015 18:43
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.

7 participants