Add documentation for AssignmentInTernaryCondition sniff #2488

Nic-Sevic · 2024-09-17T19:02:33Z

Related to #1722

/docs/CodeAnalyis/AssignmentInTernaryConditionStandard.xml

rodrigoprimo

Thanks for working on this PR, @Nic-Sevic! I left some comments for us to discuss. Please take into account that I'm not a maintainer of the project so I might have missed something or suggest something that the maintainers might disagree.

rodrigoprimo · 2024-09-17T21:30:35Z

WordPress/Docs/CodeAnalysis/AssignmentInTernaryConditionStandard.xml

@@ -0,0 +1,19 @@
+<documentation title="Assignment In Ternary Condition">


I would suggest adding the XML version before the <document> tag and also adding the xmlns:xsi and xsi:noNamespaceSchemaLocation attributes to the <document> similar to what is done in the XML files (maybe there are some that don't do that?). Example:

https://github.com/WordPress/WordPress-Coding-Standards/blob/develop/WordPress/Docs/Arrays/ArrayIndentationStandard.xml#L1-L5

rodrigoprimo · 2024-09-17T21:32:26Z

WordPress/Docs/CodeAnalysis/AssignmentInTernaryConditionStandard.xml

@@ -0,0 +1,19 @@
+<documentation title="Assignment In Ternary Condition">
+<standard>


This needs four spaces for indentation.

rodrigoprimo · 2024-09-17T21:32:45Z

WordPress/Docs/CodeAnalysis/AssignmentInTernaryConditionStandard.xml

+<documentation title="Assignment In Ternary Condition">
+<standard>
+    <![CDATA[
+        Checks that variable assignment does not occur in conditional statement of ternary. Condition must be in parentheses to be checked; Ternaries lacking parentheses around condition are skipped. If incorrect will throw: "Variable assignment found within a condition. Did you mean to do a comparison?"


And this line should have four spaces less.

👍 (not sure why I can't mark those comments as resolved)

rodrigoprimo · 2024-09-17T21:48:58Z

WordPress/Docs/CodeAnalysis/AssignmentInTernaryConditionStandard.xml

+<documentation title="Assignment In Ternary Condition">
+<standard>
+    <![CDATA[
+        Checks that variable assignment does not occur in conditional statement of ternary. Condition must be in parentheses to be checked; Ternaries lacking parentheses around condition are skipped. If incorrect will throw: "Variable assignment found within a condition. Did you mean to do a comparison?"


Checks that variable assignment does not occur in conditional statement of ternary.

Usually, the standard descriptions are quite explicit/directive. Similar to the error messages. So maybe instead of saying what the sniffs checks, I would suggest describing what the sniff expects. Something like:

"There should be no assignments in the condition part of ternary."

This might need some more polishing but should give the general idea. Checking other descriptions might help as well.

If incorrect will throw: "Variable assignment found within a condition. Did you mean to do a comparison?"

I believe that the sniff documentation usually does not include the error message displayed, so I suggest not including it here unless there is a reason to include it for this particular sniff.

Tried to clarify standard description and improve readability

Thanks for improving the description. I just realized that I misled you with the text I suggested. This sniff throws a warning and not an error, so we should not use should. Instead, we could say something like it is recommended or some other variation. Could you please update the description to account for that?

rodrigoprimo · 2024-09-17T21:52:36Z

WordPress/Docs/CodeAnalysis/AssignmentInTernaryConditionStandard.xml

+    ]]>
+    </standard>
+    <code_comparison>
+        <code title="Valid: Variable assigned with comparison in parentheses.">


This sentence seems a bit confusing to me because of the use of the word "assigned". The sniff is not about assigning the result of a ternary comparison to a variable. The sniff is triggered whether or not the result of the ternary is assigned to a variable. At the same time, the sniff is precisely checking if there is no assignment in the condition part of the ternary.

Maybe this could be rephrased to avoid this confusion? I also wonder if it would be better to include just the ternary without the echo and the assignment to variable in the examples.

tried to clarify

rodrigoprimo · 2024-09-17T22:02:51Z

WordPress/Docs/CodeAnalysis/AssignmentInTernaryConditionStandard.xml

+    <code_comparison>
+        <code title="Valid: Variable assigned with comparison in parentheses.">
+        <![CDATA[
+echo $mode = ( <em>$a == 'a'</em> ) ? 'b' : 'c';


Unless I'm missing something, I don't think that having an assignment and then echoing in the same line is necessary to illustrate a valid example and, in my opinion, adds a layer of complexity that makes it a bit harder to understand this example.

This was a misunderstanding on my part from mirroring another file; removed

rodrigoprimo · 2024-09-17T22:13:15Z

WordPress/Docs/CodeAnalysis/AssignmentInTernaryConditionStandard.xml

+        </code>
+        <code title="Invalid: Parentheses used but variable assignment instead of comparison.">
+        <![CDATA[
+echo $mode = ( <em>$a = 'a'</em> ) ? 'b' : 'c';


I wonder if we should highlight just the assignment operator (and the compison operator above)? I'm not sure to be honest. What do you think?

I think you're right, adjusted emphasis here

rodrigoprimo

Thanks for improving this PR, @Nic-Sevic. It is looking good! I left some final comments for us to discuss before I approve it.

rodrigoprimo · 2024-09-26T12:13:45Z

WordPress/Docs/CodeAnalysis/AssignmentInTernaryConditionStandard.xml

+    >
+    <standard>
+    <![CDATA[
+    Variables should not be assigned in conditional statement of ternary. 


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.

rodrigoprimo · 2024-09-26T12:16:48Z

WordPress/Docs/CodeAnalysis/AssignmentInTernaryConditionStandard.xml

+    <standard>
+    <![CDATA[
+    Variables should not be assigned in conditional statement of ternary. 
+    Condition must be in parentheses to be checked; ternaries lacking parentheses around condition are skipped. 


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.

rodrigoprimo · 2024-09-26T12:28:20Z

WordPress/Docs/CodeAnalysis/AssignmentInTernaryConditionStandard.xml

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


Suggested change

$mode = ( $a == 'a' ) ? 'b' : 'c';

$mode = ( $a == '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.

rodrigoprimo · 2024-09-26T12:32:46Z

WordPress/Docs/CodeAnalysis/AssignmentInTernaryConditionStandard.xml

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


Suggested change

$mode = ( $a == 'a' ) ? 'b' : 'c';

$mode = ( $a === '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.

Add: documentation for AssignmentInTernaryCondition sniff

f7975dc

jrfnl mentioned this pull request Sep 17, 2024

Add documentation for each sniff #1722

Open

61 tasks

rodrigoprimo added Type: Documentation Component: Core labels Sep 17, 2024

rodrigoprimo reviewed Sep 17, 2024

View reviewed changes

rodrigoprimo added the Status: Awaiting feedback label Sep 17, 2024

fix: syntax issues, clarify desc and code comparison

9d85cf6

rodrigoprimo removed the Status: Awaiting feedback label Sep 25, 2024

rodrigoprimo reviewed Sep 26, 2024

View reviewed changes

rodrigoprimo added the Status: Awaiting feedback label Sep 26, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add documentation for AssignmentInTernaryCondition sniff #2488

Add documentation for AssignmentInTernaryCondition sniff #2488

Nic-Sevic commented Sep 17, 2024

rodrigoprimo left a comment

rodrigoprimo Sep 17, 2024

Nic-Sevic Sep 17, 2024

rodrigoprimo Sep 26, 2024

rodrigoprimo Sep 17, 2024

Nic-Sevic Sep 17, 2024

rodrigoprimo Sep 26, 2024

rodrigoprimo Sep 17, 2024

Nic-Sevic Sep 17, 2024

rodrigoprimo Sep 26, 2024

rodrigoprimo Sep 17, 2024 •

edited

Loading

Nic-Sevic Sep 17, 2024 •

edited

Loading

rodrigoprimo Sep 26, 2024

rodrigoprimo Sep 17, 2024

Nic-Sevic Sep 17, 2024

rodrigoprimo Sep 17, 2024

Nic-Sevic Sep 17, 2024

rodrigoprimo Sep 17, 2024

Nic-Sevic Sep 17, 2024

rodrigoprimo left a comment

rodrigoprimo Sep 26, 2024

rodrigoprimo Sep 26, 2024

rodrigoprimo Sep 26, 2024

rodrigoprimo Sep 26, 2024

		@@ -0,0 +1,19 @@
		<documentation title="Assignment In Ternary Condition">

		@@ -0,0 +1,19 @@
		<documentation title="Assignment In Ternary Condition">
		<standard>

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

	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.

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

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

Add documentation for AssignmentInTernaryCondition sniff #2488

Are you sure you want to change the base?

Add documentation for AssignmentInTernaryCondition sniff #2488

Conversation

Nic-Sevic commented Sep 17, 2024

rodrigoprimo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rodrigoprimo Sep 17, 2024 • edited Loading

Choose a reason for hiding this comment

Nic-Sevic Sep 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rodrigoprimo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rodrigoprimo Sep 17, 2024 •

edited

Loading

Nic-Sevic Sep 17, 2024 •

edited

Loading