-
Notifications
You must be signed in to change notification settings - Fork 731
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.
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
get-debug-type.xml Make the example executable + tags #3755
Conversation
echo get_debug_type(new class implements A {}) . PHP_EOL | ||
, get_debug_type(new class implements A,B {}) . PHP_EOL | ||
, get_debug_type(new class extends C {}) . PHP_EOL | ||
, get_debug_type(new class extends C implements A {}) . PHP_EOL | ||
; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The indent here seems to be slightly off
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I would leave that as is; the leading commas are somwhat uncommon, and someone reading it, might suggest to change it again (we really should have a CS for the examples in the manual).
Maybe change the dots to commas, though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No there was an actual issue with the indent which was mixing tabs and spaces
@@ -142,6 +143,9 @@ | |||
<programlisting role="php"> | |||
<![CDATA[ | |||
<?php | |||
|
|||
namespace Foo; | |||
|
|||
echo get_debug_type(null) . PHP_EOL; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Small nit, but while you are changing something here, I would replace PHP_EOL
with "\n"
. That constant should be used only when parsing something.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That constant should be used only when parsing something.
And possibly not even then. :)
If touching this line, I would suggest to replace the dot with a comma (no need to concatenate the string before echoing).
I'm fine with the example changes, but it seems to me we should establish a coding standard, and automatically check that (so we don't go back and forth). A while ago I found https://github.com/php/doc-base/blob/master/scripts/qa/check-examples.php which currently applies PHP's linting to the examples; maybe that could be expanded (or changed) to run some tool (maybe PHP_CodeSniffer or PHP CS Fixer). |
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM with one small nit.
Discussion about CS taken to mailing list: https://news-web.php.net/php.doc/969388442
I'll also add a note about using ,
instead of .
with echo
to the style guide.
<literal>"class@anonymous"</literal> or parent class name/interface name if the class extends another class or implements an interface e.g. <literal>"Foo\Bar@anonymous"</literal> | ||
<literal>"class@anonymous"</literal> or parent class name/interface name if | ||
the class extends another class or implements an interface e.g. <literal>"Foo\Bar@anonymous"</literal> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should undo this re-wrapping just to avoid confusing translators.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thank you!
No description provided.