Enable runnable (WASM) examples for language.functions section#5463
Enable runnable (WASM) examples for language.functions section#5463AllenJB wants to merge 2 commits intophp:masterfrom
Conversation
| echo "Example function.\n"; | ||
| return $retval; | ||
| } | ||
| ?> |
There was a problem hiding this comment.
Removing the closing ?> is needless churn. Given how the examples sit within the documentation, I personally also feel that the ?> is a nice “example ends here” indicator.
There was a problem hiding this comment.
I've been removing these because I've seen new developers always including them (and occasionally running into the subsequent issues) and I think one reason they may do this is because the manual examples do.
Additionally I would note that the coding style for examples in doc-base says to follow PER-CS, which also says that ?> MUST be omitted from files containing only PHP (I generally consider each example to be a "file" for this purpose, particularly where the examples is runnable)
There was a problem hiding this comment.
Additionally I would note that the coding style for examples in doc-base says to follow PER-CS, which also says that
?>MUST be omitted from files containing only PHP
The documentation naturally cannot follow any coding style in its entirety, because it needs to showcase everything that's possible in PHP. I would consider it a soft-target of “use the brace placement” or “use the whitespace” as defined in PER-CS, which is what does the heavy lifting of making code look uniform.
In fact almost all of the examples in the documentation already violate a SHOULD from PSR-1 because they mix declarations and side-effects. Due to the echos I would also consider them to not be a file “containing only PHP”.
In any case, PRs should be focused on one thing. A PR titled “enable runnable examples” should not make changes that have no effect on whether the example is runnable or not.
There was a problem hiding this comment.
Violating mixed declarations and side-effects is obviously unavoidable (particularly with runnable examples). I don't think this is a good counter-example here. Trailing ?> is not unavoidable.
Many examples obviously don't follow PER-CS in many ways for legacy reasons, but if it's the documented style standard then surely examples should be updated where this is noticed and reasonable, in the same way I've been asked to correct simpara vs para (and other) xml style violations, including outside the scope of changes I was otherwise making.
If the manual code style is to ignore this PER-CS rule / always include trailing ?>, I would suggest this is noted in the cs-for-examples docs. I think this is a significant and not necessarily expected departure from PER-CS and other contributors are likely to run into the same issue.
Girgias
left a comment
There was a problem hiding this comment.
I don't think removing the ?> is worthwhile churn, especially as this is adding kinda unnecessary work for translations.
Related: #4799