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.

Sign up for GitHub

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

Add documentation for AssignmentInTernaryCondition sniff #2488

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from
Open

Add documentation for AssignmentInTernaryCondition sniff #2488

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f7975dc
Add: documentation for AssignmentInTernaryCondition sniff
Nic-Sevic Sep 17, 2024
9d85cf6
fix: syntax issues, clarify desc and code comparison
Nic-Sevic Sep 17, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 24 additions & 0 deletions WordPress/Docs/CodeAnalysis/AssignmentInTernaryConditionStandard.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
<?xml version="1.0"?>
<documentation xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
xsi:noNamespaceSchemaLocation="https://phpcsstandards.github.io/PHPCSDevTools/phpcsdocs.xsd"
title="Assignment In Ternary Condition"
>
<standard>
<![CDATA[
Variables should not be assigned in conditional statement of ternary.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
Variables should not be assigned in conditional statement of ternary.
Variables should not be assigned in the condition of a ternary operator.

I believe it is more common to use "condition" instead of "conditional statement" to refer to the first expression of a ternary, but I could be wrong. Reference: https://en.wikipedia.org/wiki/Ternary_conditional_operator. I found no mention of "condition" or "conditional statement" in the php.net page about ternaries: https://www.php.net/manual/en/language.operators.comparison.php. We might need to update the text for the valid and invalid examples as well.

Also, the sentence reads better to me if the article "a" is added before "ternary". That being said, I'm not a native speaker, so let me know if that is not the case.

Condition must be in parentheses to be checked; ternaries lacking parentheses around condition are skipped.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
Condition must be in parentheses to be checked; ternaries lacking parentheses around condition are skipped.
The condition must be in parentheses to be checked; ternaries lacking parentheses around the condition are skipped.

Similar to the above, the sentence reads better to me if we add the article "the", but feel free to ignore this comment if that is not the case.

]]>
</standard>
<code_comparison>
<code title="Valid: Conditional statement in parentheses and does not include variable assignment.">
<![CDATA[
$mode = ( $a <em>==</em> 'a' ) ? 'b' : 'c';
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
$mode = ( $a <em>==</em> 'a' ) ? 'b' : 'c';
$mode = ( $a <em>==</em> 'a' ) ? 'b' : 'c';

Just a nitpick, but usually, the code examples are not indented and should have no spaces. So, the initial version was correct. It doesn't have a noticeable impact in this case, and this is just a single line, but for multiple line code samples, it makes a difference.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
$mode = ( $a <em>==</em> 'a' ) ? 'b' : 'c';
$mode = ( $a <em>===</em> 'a' ) ? 'b' : 'c';

This is another nitpick, but I suggest using a strict comparison instead of a loose comparison in the valid example so that it follows coding best practices.

]]>
</code>
<code title="Invalid: Conditional statement in parentheses but includes variable assignment instead of comparison.">
<![CDATA[
$mode = ( $a <em>=</em> 'a' ) ? 'b' : 'c';
]]>
</code>
</code_comparison>
</documentation>
Loading