Repository navigation
Conversation
rodrigoprimo
left a comment
rodrigoprimo
left a comment
There was a problem hiding this comment.
Thanks for working on this PR, @gogdzl! I'm not a maintainer of the project so take my review with a grain of salt. That being said, I left some comments based on my own experience creating PHPCS and WPCS sniffs.
| Debug code should not normally be used in production. | ||
|
|
||
| Typically, this rule verifies if function calls to the PHP native `error_log()`, `var_dump()`, `var_export()`, `print_r()`, `trigger_error()`, `set_error_handler()`, `debug_backtrace`, `debug_print_backtrace` and `wp_debug_backtrace_summary()` functions are present in the code. |
There was a problem hiding this comment.
| Typically, this rule verifies if function calls to the PHP native `error_log()`, `var_dump()`, `var_export()`, `print_r()`, `trigger_error()`, `set_error_handler()`, `debug_backtrace`, `debug_print_backtrace` and `wp_debug_backtrace_summary()` functions are present in the code. | |
| This rule verifies if calls to the `error_log()`, `var_dump()`, `var_export()`, `print_r()`, `trigger_error()`, `set_error_handler()`, `debug_backtrace()`, `debug_print_backtrace()` and `wp_debug_backtrace_summary()` functions are present in the code. |
I'm suggesting the removal of the PHP native part as wp_debug_backtrace_summary() is not a PHP native function.
There was a problem hiding this comment.
Maybe instead of verifies, we could use warns to more accurately describe what the rule does? Just thinking out loud here.
| > | ||
|
|
||
| Debug code should not normally be used in production. |
There was a problem hiding this comment.
I know that this is the warning message displayed by the sniff, but I believe that the documentation (and maybe also the sniff messages) usually only uses should/should not when describing an error. Since this is a warning, maybe we could say that it is recommended or something like that?
There was a problem hiding this comment.
@rodrigoprimo This is incorrect.
For the record - "should"/"should not" translates to warnings, "must"/"must not" translates to errors.
This is based on an official RFC about this type of terminology: https://www.rfc-editor.org/rfc/rfc2119
There was a problem hiding this comment.
Thanks for the reminder. I have learned that since I left this comment, but I must say that sometimes I mix the two. I suggest adding a note about this to the "Guidelines for the documentation and the code samples" section of #1722. What do you think?
There was a problem hiding this comment.
I don't think an issue is the right place for that and more than anything, this RFC applies to the standards as described in the handbook, with WPCS following, not the other way around.
There was a problem hiding this comment.
I suggest this because I think it is the kind of information that will be useful for those who work on creating WPCS sniff documentation.
There was a problem hiding this comment.
Agreed, but I just don't think the issue is the right place. Maybe a CONTRIBUTING file for the wpcs-docs repo ? Maybe the CONTRIBUTING file for this repo ?
There was a problem hiding this comment.
I agree that a CONTRIBUTING file is a better place for this.
Assuming this applies to PHPCS as well, maybe we should add a note on when to use must/should to its CONTRIBUTING.md file? We could add it to the "Writing Sniff Documentation" section. I know this doesn't apply only to sniff documentation but also to the error/warning messages generated by sniffs, but I'm not sure where else to add it.
Then, in the WPCS CONTRIBUTING file, we could address #2462 by creating a section pointing to "Writing Sniff Documentation" in the PHPCS repository and also add a new sub-section under "Considerations when writing sniffs".
As for the wpcs-docs repo, it doesn't have a CONTRIBUTING file, and I'm not sure where this guidance would fit there.
How does this sound?
There was a problem hiding this comment.
The RFC standard I referenced is primarily about the meaning of certain phrases in standards.
PHP_CodeSniffer and WPCS are not the standards. They are tools to enforce standards. The standards themselves are described in the WP Core handbook (for WPCS), the PEAR project or the FIG published PSRs.
And each of those has to decide for themselves whether they want to follow the RFC. After all, that's not a given perse.
So, more than anything, this is something which should be in the contributing guides of the verbal standards.
And both sniffs as well as the XML docs for those sniffs should follow the lead of the actual standards.
There was a problem hiding this comment.
Thanks for clarifying the parts I was getting wrong.
That said, I think there's still value in having a brief note in the WPCS CONTRIBUTING file to help contributors be consistent when writing XML docs and sniff messages. Not to define the convention in the WPCS repository, but to remind contributors to mirror the language used in the standard (e.g., "must" for errors, "should" for warnings). Especially considering that, unless I'm missing something, there is no clear place at the moment to add this instruction in the wpcs-docs repository.
Would that be acceptable?
| ]]> | ||
|
|
||
|
There was a problem hiding this comment.
Why not is in between parenthesis?
Also, I wonder if the message should be more generic and mention debug code/functions instead of singling out var_dump()? I'm ok with a single example with just var_dump().
|
@gogdzl, I was just wondering if you'll have a chance to finish this off in the near future. It would be great if this PR could be included in the next WPCS release. If you haven't got time or lost interest, please let us know and we'll see if we can find someone to take over. Thanks! |
|
@gogdzl, please let us know within a week if you are still interested in finishing this PR. If we don't hear back from you, we will presume you don't have time, and we will see if we can find someone else to take over and finish it. Thanks for your work so far! |
3 participants
Related to #1722