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.

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

Properly support union types #30

Closed
wants to merge 9 commits into from
Closed

Conversation

cmb69
Copy link
Member

@cmb69 cmb69 commented Oct 1, 2020

This is the cleaner alternative to PR #25, which supports nested
<type> elements, and renders them as union types in PHP 8 syntax.


This is just a quick and dirty POC for now, in the hope to get that going. Assuming our docbook would support nested types, the following patch to doc-en

Index: strpos.xml
===================================================================
--- strpos.xml	(revision 350727)
+++ strpos.xml	(working copy)
@@ -11,7 +11,7 @@
   <methodsynopsis>
    <type>int</type><methodname>strpos</methodname>
    <methodparam><type>string</type><parameter>haystack</parameter></methodparam>
-   <methodparam><type>mixed</type><parameter>needle</parameter></methodparam>
+   <methodparam><type class="union"><type>string</type><type>int</type></type><parameter>needle</parameter></methodparam>
    <methodparam choice="opt"><type>int</type><parameter>offset</parameter><initializer>0</initializer></methodparam>
   </methodsynopsis>
   <para>

would be rendered like
Screenshot 2020-10-01 133429
Note that the predefined types are now rendered as links, which might not be the worst idea generally.

This is the cleaner alternative to PR php#25, which supports nested
`<type>` elements, and renders them as union types in PHP 8 syntax.
@cmb69 cmb69 mentioned this pull request Oct 1, 2020
@salathe
Copy link
Contributor

salathe commented Oct 1, 2020

Thanks for pursuing this @cmb69.

Note that the predefined types are now rendered as links, which might not be the worst idea generally.

It would be good to see a given type rendered the same regardless of whether it is a standalone type or part of a union type in the prototype.

At least for the short term, make sure to pass the type through Package_PHP_XHTML::format_type_if_object_or_pseudo_text() for consistency. That should just be a matter of adding 'type' => 'format_type_if_object_or_pseudo_text', to Package_PHP_XHTML::$mytextmap['type'].

I don't think I'd object if that method was phased out (and so all types became links), but that's probably a task best left for a later date (or at least a separate PR).

Assuming our docbook would support nested types...

I've patched our old DocBook DTD with a quick change to support union types (not intersection) in php/doc-base@82d74e6 so the above strpos() example should happily configure.php now.

@cmb69
Copy link
Member Author

cmb69 commented Oct 6, 2020

I don't think I'd object if that method was phased out (and so all types became links), but that's probably a task best left for a later date (or at least a separate PR).

I agree that this should kept separate. It might be useful to do that in advance, though, since it would simplify this PR; currently, the link to built-in types are only suppressed in methodsynopses, but not elsewhere.

Anyhow, checking php/doc-en#151 shows that there's more work to be done (at least union return types are not properly rendered yet).

We also do that for the PDF package, although we haven't touched that
code otherwise.  That might need to be done, despite the PDF rendering
being broken anyway.
phpdotnet/phd/Package/PHP/XHTML.php Outdated Show resolved Hide resolved
phpdotnet/phd/Package/PHP/XHTML.php Show resolved Hide resolved
As suggested by Nikita.
This is all done in `format_type_methodsynopsis_text()` and
`format_methodsynopsis()` already.
@cmb69
Copy link
Member Author

cmb69 commented Oct 29, 2020

Applied as 4d91cb2.

@cmb69 cmb69 closed this Oct 29, 2020
@cmb69 cmb69 deleted the cmb/union-types-2 branch October 29, 2020 16:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants