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

Jump to bottom

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 1 commit
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
19 changes: 19 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,19 @@
<documentation title="Assignment In Ternary Condition">
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

added

Copy link
Collaborator

Choose a reason for hiding this comment

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

👍

<standard>
Copy link
Collaborator

Choose a reason for hiding this comment

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

This needs four spaces for indentation.

Copy link
Author

Choose a reason for hiding this comment

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

fixed

Copy link
Collaborator

Choose a reason for hiding this comment

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

👍

<![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?"
Copy link
Collaborator

Choose a reason for hiding this comment

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

And this line should have four spaces less.

Copy link
Author

Choose a reason for hiding this comment

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

fixed

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

@rodrigoprimo rodrigoprimo Sep 17, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Author

@Nic-Sevic Nic-Sevic Sep 17, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Tried to clarify standard description and improve readability

Copy link
Collaborator

Choose a reason for hiding this comment

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

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?

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

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

tried to clarify

<![CDATA[
echo $mode = ( <em>$a == 'a'</em> ) ? '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.

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.

Copy link
Author

Choose a reason for hiding this comment

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

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

]]>
</code>
<code title="Invalid: Parentheses used but variable assignment instead of comparison.">
<![CDATA[
echo $mode = ( <em>$a = 'a'</em> ) ? '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.

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?

Copy link
Author

Choose a reason for hiding this comment

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

I think you're right, adjusted emphasis here

]]>
</code>
</code_comparison>
</documentation>
Loading