Improve parameter names in ext/xmlwriter #6202
Conversation
ext/xmlwriter/php_xmlwriter.stub.php
Outdated
|
|
||
| function xmlwriter_start_dtd_element(XMLWriter $xmlwriter, string $qualifiedName): bool {} | ||
| function xmlwriter_start_dtd_element(XMLWriter $writer, string $qualified_name): bool {} |
There was a problem hiding this comment.
As these are aliased with XmlWriter::startDtdElement() etc and have common documentation (https://www.php.net/manual/en/function.xmlwriter-start-dtd-element.php), I believe we need the parameter names to match as well. Is that correct @cmb69?
There was a problem hiding this comment.
Oops, I was the one who told Dik that it's possible to use different names for the procedural and the OO API (since it has no technical limitation). Unfortunately, I didn't think about the documentation :/
There was a problem hiding this comment.
As it is now, the procedural and OOP API have individual <methodsynopsis> elements, so this is not an issue for the documentation. I am generally not sure that having different parameter names for dual APIs is a good idea, though, since that may make transitioning from procedural to OOP slightly harder (the other way round is equally harder, but likely not a relevant issue).
There was a problem hiding this comment.
@cmb69 The issue I had in mind here is that parameter names are also referenced in the body of the documentation, not just the method synopsis. There wouldn't be a unique parameter name to use in that case (though I guess using either one wouldn't be terrible either).
There was a problem hiding this comment.
Oh, indeed, that wouldn't be nice.
There was a problem hiding this comment.
I think it is worth considering to simply always use
$snake_casestyle parameter names inside php-src.
Does this not go against the general direction to make internals and userland more alike, blend more naturally? At some point, internal APIs may even be written in PHP. In that scenario, having internal APIs look less alien from a userland perspective seems relevant...
(Of course, always using $camelCase in php-src is also an option, I just suggested snake_case as it seems to be currently predominant.)
I prefer that proposal. I agree that the procedural APIs are of lesser importance. Making the OO APIs come out right is the main thing. If the procedural versions also come out right, that is a nice to have.
There was a problem hiding this comment.
(Of course, always using $camelCase in php-src is also an option, I just suggested snake_case as it seems to be currently predominant.)
Just to make sure I understand your proposal correctly:
- Pure OO APIs: camelCase
- Dual APIs: camelCase
- Pure procedural APIs: snake_case
There was a problem hiding this comment.
Yes, either that, or pick one of camelCase/snake_case for everything.
There was a problem hiding this comment.
I think keeping the snake_case for pure procedural is nice. It makes the parameter naming style match the existing snake_case function names. Also, most APIs are in that style already.
Using camelCase for everything else nicely matches the style of most userland code.
There was a problem hiding this comment.
Just pushed an update to apply Nikita's proposal.
dtakken
force-pushed
the
parameter_names_xmlwriter
branch
from
6315aee to
83afd1d
Compare
dtakken
force-pushed
the
parameter_names_xmlwriter
branch
from
83afd1d to
456f5df
Compare
|
I just pushed an update that includes review feedback from @nikic. |
|
|
||
| function xmlwriter_write_element(XMLWriter $xmlwriter, string $name, ?string $content = null): bool {} | ||
| function xmlwriter_write_element(XMLWriter $writer, string $name, ?string $content = null): bool {} |
There was a problem hiding this comment.
Aha! I see why content was used for attributes! While it is's a good term for describing what an element contains, it's not the best in case of attributes.
There was a problem hiding this comment.
I understand your preference for $value for attributes. I would be OK with using $value for attributes while keeping $content elsewhere. Are you OK with that as well @nikic?
There was a problem hiding this comment.
Yeah, I'm okay with making that distinction.
There was a problem hiding this comment.
Ok, I just pushed these last few changes.
dtakken
force-pushed
the
parameter_names_xmlwriter
branch
from
456f5df to
803deee
Compare
As part of php/php-tasks/issues/23