Reformat methodsynopsis whitespaces

[kocsismate]

This PR removes some unnecessary whitespace characters from the methodsynopses.

So

openssl_csr_sign ( mixed $csr , mixed $cacert , mixed $priv_key , int $days , array $configargs = ? , int $serial = 0 ) : resource

becomes

openssl_csr_sign (mixed $csr , mixed $cacert , mixed $priv_key , int $days , array $configargs = ? , int $serial = 0 ): resource

Unfortunately, a few unnecessary whitespace characters still remain, and I couldn't yet find out how to remove them as well :/ For me, it looks like some whitespace characters from the XML get rendered directly to the HTML (e.g. around the ,). Is it possible?

[cmb69]

Yes, indeed, that are the whitespace nodes which are rendered verbatim:

phd/phpdotnet/phd/Render.php

Lines 161 to 169 in 785fec0

	case \XMLReader::WHITESPACE: /* {{{ */
	case \XMLReader::SIGNIFICANT_WHITESPACE:
	/* WS is always WS */
	$retval = $r->value;
	foreach($this as $format) {
	$format->appendData($retval);
	}
	break;
	/* }}} */

Just skipping this would yield the desired results for the methodsynopses, but would likely break other stuff. :(

[kocsismate]

@cmb69 Do you see any chance to make this work somehow? If there is not any, I'll close this PR.

[cmb69]

 phpdotnet/phd/Render.php | 1 +
 1 file changed, 1 insertion(+)

diff --git a/phpdotnet/phd/Render.php b/phpdotnet/phd/Render.php
index a3d3413..df01e6a 100644
--- a/phpdotnet/phd/Render.php
+++ b/phpdotnet/phd/Render.php
@@ -161,6 +161,7 @@ class Render extends ObjectStorage
                 case \XMLReader::WHITESPACE: /* {{{ */
                 case \XMLReader::SIGNIFICANT_WHITESPACE:
                             /* WS is always WS */
+                if ($this->STACK[$r->depth - 1] === 'methodsynopsis') break;
                 $retval  = $r->value;
                 foreach($this as $format) {
                     $format->appendData($retval);

on top of this might already solve the whitespace issue for functions; methods would need additional care (because of the modifiers).

Anyhow, it might be a good idea to add more tests (and to fix the test suite if necessary).

[cmb69]

Actually, we may want to show something like

[kocsismate]

Thank you for the help! I'll come back to this task as soon as I have some spare time for it.

Actually, we may want to show something like

is it the same thing what was proposed here: php/php-src#6367 (comment) ? When would you break the lines? Always? Or just for functions with a too long parameter list?

[cmb69]

For simplicity, it might make sense to have each parameter on its own line.

[kocsismate]

I forgot how to properly generate the docs locally, but I used php render.php, and I get the following outputs:

public static FFI\CData FFI::addr(
     FFI\CData &$ptr
)

public void DOMDocument::normalizeDocument()

mixed json_decode(
     string $json,
     bool|null $associative = null,
     int $depth = 512,
     int $flags = 0
)

(I'm not sure why the type precedes the method name)

[kocsismate]

@cmb69 I didn't manage to understand how to run the test suite (or even how to properly generate the docs). Could you give me some pointers? Is the current format what you imagined?

[cmb69]

I never ran the test suite, so wouldn't know how to do it. How to build and render the docs is described in the tutorial on http://doc.php.net/, but apparently this site is down now. :(

Anyhow, the output looks good to me, but the return types need to go at the end. This is accomplished by putting the output into $this->cchunk["methodsynopsis"]["returntypes"], but it seems that is no longer filled, but instead the return types are rendered when they are read. Would need some debbuging to see what's going on.

[kocsismate]

Anyhow, the output looks good to me, but the return types need to go at the end.

Now, I used the following command to build the docs from doc-en (I found it in my zsh history):

 php ../phd/render.php --docbook ../doc-base/.manual.xml --package PHP --format xhtml

I think last time I didn't specify the format (probably neither the other arguments...), and it might be the reason why the return types were misplaced as well as the pages missed any formatting. So now, I have pretty decent looking function/method/class reference pages. However, the new formatting looks a bit strange in case of class reference pages - at least at the first sight.

I'm attaching a few screenshots:

[cmb69]

I think last time I didn't specify the format (probably neither the other arguments...), and it might be the reason why the return types were misplaced as well as the pages missed any formatting.

Ah, right, for non XHTML based formats the return types have not been fixed. Not sure what else is broken for these formats, and whether we really want to keep them.

However, the new formatting looks a bit strange in case of class reference pages - at least at the first sight.

That may require quite some manual scrolling, but is IMHO better than what we have now:

It might be reasonable to supress parameters (and return types) for the class pages. Anyhow, I think it's best to write to the docs ML to ask for feedback.

Thank you very much for your work!

[salathe]

I am a huge fan of the whitespace removal around parentheses, commas, etc.. Big, big thumbs up for that.

I am not a fan of making the prototype, for every single function/method that has parameters, span across multiple lines. This can make class synopses almost unusable, IMO. For example, here is my screen's worth of DateTime methods:

I don't know about anyone else, but my brain can't make quick sense of that.

At the opposite end of the scale, small function/method prototypes also look awful, IMO.

Note, my key issue is spreading the prototype across multiple lines where you wouldn't normally do that in your own code style. For prototypes where it would be clearer to span multiple lines (e.g. many parameters, or lots of parameter type declarations particularly with union types), I'm all for doing that: increasing the readability of the manual pages just as you would for actual code.

If, for simplicity's sake, the choice is either to spread every single prototype (with any parameters) across multiple lines deliberately, or to not do that; personally, I would recommend staying with the latter.

[cmb69]

I'm not in favor of not splitting long argument lists, e.g. https://www.php.net/manual/en/function.snmp3-set.php is awful in my opinion; also see the SimpleXMLElement class synopsis above.

Maybe we could only split long argument lists?

[iluuu1994]

Splitting makes long signatures much more readable but the inverse is also true. Most code formatting tools only split long lines, maybe 80-100 characters.

Maybe we could only split long argument lists?

I'd check the whole signature. If the function name is very long, it might still make sense to split a short argument list. I'm not familiar with this code so I'm not sure how simple that is to implement.

[cmb69]

I'm not familiar with this code […]

I'm afraid almost nobody is familiar with this code.

[kocsismate]

I'd check the whole signature. If the function name is very long, it might still make sense to split a short argument list. I'm not familiar with this code so I'm not sure how simple that is to implement.

I'm not keen on counting characters... However, I would be ok to either:

• take one step back, and only remove the unnecessary whitespaces without adding new line breaks
• add newlines based on the number of parameters. E.g. parameter lists with 4 or more parameters could be broken into multiple lines. I know it's not a perfect solution for the problem, but this could work just well probably in 80-90% of the cases. This also seems fairly simple to implement.

What do you think?

[cmb69]

Yes, we need to keep implementation complexity low. If splitting by number of arguments helps with that, this is preferable. Otherwise I'd prefer to split by number of characters.

Addionally, we may consider to add white-space:pre and maybe overflow-x:auto, but that isn't a PhD issue.

[kocsismate]

I managed to implement something which looks much-much better, see the screenshots attached. I have to admit though that the implementation is a bit hacky.

From now on, this PR only breaks function signatures with at least 4 parameters into multiple lines. I think this is a reasonably good heuristics to find long signatures.

I've just realized that "function" is missing from all signatures, I guess due to brevity. In my opinion, we could afford to display it as well as the ending ;/{} after my changes, so that all method signatures would become valid PHP syntax.

[kocsismate]

I've just rebased to current master

[cmb69]

@salathe, would the overhauled PR resolve your concerns?

[salathe]

From now on, this PR only breaks function signatures with at least 4 parameters into multiple lines. I think this is a reasonably good heuristics to find long signatures.

Thanks. I think "reasonably good" is good enough for now. 👍

@salathe would the overhauled PR resolve your concerns?

See above. 😸

I've just realized that "function" is missing from all signatures, I guess due to brevity. In my opinion, we could afford to display it as well as the ending ;/{} after my changes, so that all method signatures would become valid PHP syntax.

There are other things missing (like class before the class name in class synopses) or otherwise not valid (e.g. we use ? for optional function/method parameters without an initializer). But, for what it's worth, any steps in that direction are okay by me.

[kocsismate]

Thanks. I think "reasonably good" is good enough for now. 👍

Thanks! :)

There are other things missing (like class before the class name in class synopses) or otherwise not valid (e.g. we use ? for optional function/method parameters without an initializer). But, for what it's worth, any steps in that direction are okay by me.

Yeah, I'd like to propose fixes for these after this PR is merged. :)