RFC: Writing parameter names in @param annotations (accepted)

Jan Tvrdík

Writing parameter names in @param annotations

This RFC proposes to update the current coding standards to always write parameter name in @param annotations.

Current state

/**
 * @param  string
 */
public function getParameter($key)

Proposed state

/**
 * @param  string $key
 */
public function getParameter($key)

Rationale

The phpDoc is there to serve users of Nette Framework. It is there to make their life easier. Unfortunately the syntax currently used by Nette Framework is

All of those problems are very unlikely to change.

Open questions

How should we align the parameter names? Alignment (in general) increases code readability but decreases diff readability.

Last edited by Jan Tvrdík (2014-11-08 17:50)

David Matějka

Phpstorm support is partially solved by this plugin

Šaman

There is one big disadvantage – possible mismatch that 2 names. Thats reason why i dont duplicate name of param, nor type of param if param is class/interface.

<?php
# I rather dont duplicate parameter name in anotation
/**
 * @param  string  $param
 */
public function getParameter($par)
{
	return $par;
}

# And dont duplicate parameter type in anotation too
/**
 * @param  IIdentiry  $user
 */
public function getParameter(Identity $user)
{
	# …
}
?>

Of course if situation need that, do it. But usually not duplicate anything.

<?php
# Now i need specify parameter name, or write both of them to anotation. But it is exception.
/**
 * @param  string  $foo
 */
public function getParameter(Identity $user, $foo)
{
	# …
}
?>

Last edited by Šaman (2014-11-01 15:03)

Filip Procházka

I hate useless metadata duplication, but the rest of the world does it in the proposed way, and it may be stupid, but it's not THAT big of a deal to adapt.

So I'm voting +1 to this RFC.

To answer your question, I suggest not using any alignment at all, because when you start aligning types and variables, you have to keep changing the alignment, because when you add a parameter, whose type is longer than your standard scalar, and you have to change entire phpdoc.

/**
 * @param  string $key
 */
public function something($key) {

and then…

/**
 * @param  string            $key
 * @param  ParameterTypehint $value
 */
public function something($key, ParameterTypehint $value) {

versus “just not caring”

/**
 * @param string $key
 * @param ParameterTypehint $value
 */
public function something($key, ParameterTypehint $value) {

Also the diff looks nicer.

Last edited by Filip Procházka (2014-11-01 16:55)

Milo

I'm sorry, but –1 from me.

I don't like the names duplication. It is just unnecessary information and error prone. Everyone understand meaning at first glance even without names. And moreover, @matej21 's plugin works fine.

If IDEs want to support phpDoc, they should support whole specification. Parameter name is only not recommended.

In one case, I would undestand using the name. If you want to skip some parameters. For example:

class C
{
	/**
	 * @param  string $some
	 */
	public function validate(Typehint $a, Typehint $b, $some);
}

Tomáš Votruba

+1 for RFC
-1 for alignment (baby steps)

Last edited by Tomáš Votruba (2014-11-01 15:27)

Jan Tvrdík

Phpstorm support is partially solved by this plugin

The plugin is great but most people do not have it installed. And most importantly – phpDoc should serve users not the other way around.

There is one big disadvantage – possible mismatch that 2 names.

That is why you should always use refactoring when renaming variables.

I actually think that this RFC makes the phpDoc not more but less fragile (prune to errors) if the refactoring tools are used properly.

If IDEs want to support phpDoc, they should support whole specification.

They have very little motivation implementing this because it is officially wrong and used by nobody. And that is not true just for PhpStorm but for all tools which work with phpDoc.

Last edited by Jan Tvrdík (2014-11-01 16:07)

Felix

+1 for RFC

Milanov

+1 for RFC
–1 for alignment (due to diff and blame readability)

Last edited by Milanov (2014-11-01 17:11)

MartinitCZ

- 1 for RFC
– 1 for alignment

Last edited by MartinitCZ (2014-11-08 20:02)

kukulich · +1 for RFC Without alignment *Last edited by kukulich (2014-11-01 18:11)*

+1 for RFC
Without alignment

Last edited by kukulich (2014-11-01 18:11)

Patrik Votoček

vote –1 for RFC (I don't like "metadata/name"duplication)
vote –1 for alignment (I don't like it because diffs after adding longest parameter)

Last edited by Patrik Votoček (2014-11-01 18:10)

nAS

+1 for RFC
without alignment

Milo

Jan Tvrdík wrote:
And most importantly – phpDoc should serve users not the other way around.

Agree. But for me it means, keep phpDoc simple as possible.

I actually think that this RFC makes the phpDoc not more but less fragile (prune to errors) if the refactoring tools are used properly.

You must use refactoring tools properly to keep it less fragile. Without arg names, you do not need to use such tools.

If IDEs want to support phpDoc, they should support whole specification.

They have very little motivation implementing this because it is officially wrong and used by nobody.

Maybe better way is fix the phpDoc specification.

And that is not true just for PhpStorm but for all tools which work with phpDoc.

Which tools? Did you do some research/tests with other tools or IDEs?

Milo

When we are talking about it, does anybody know why is the argument name requested by phpDoc? Maybe there is a reason.

enumag · +1 for RFC, without alignment *Last edited by enumag (2014-11-01 21:19)*

+1 for RFC, without alignment

Last edited by enumag (2014-11-01 21:19)

Jan Tvrdík

You must use refactoring tools properly to keep it less fragile. Without arg names, you do not need to use such tools.

My point is that it does not change the current workflow. You can either use refactoring tool (which will handle it automatically) or manually find all occurrences of the variable name and update them.

Did you do some research/tests with other tools or IDEs?

No, it was a general statement. Once you are in an extreme minority you are always at risk of being ignored, not supported, overlooked.

does anybody know why is the argument name requested by phpDoc?

Generally I think it is much better to have no type information than to have wrong type information. If you change method signature and forget to update phpDoc, the “position based system” will most likely give you wrong types but the “name based system” will probably give you no types at all.

Last edited by Jan Tvrdík (2014-11-02 01:23)

tiso

+1 for RFC
Argument: proposed state makes error (different function declarations vs. doc comment) more visible.
aligment: no opinion

David Grudl

I propose to update this too:

	/**
	 * @var string
	 */
	public $name;

To:

	/**
	 * @var string $name
	 */
	public $name;

Jan Tvrdík

@DavidGrudl Feel free to write your own RFC with this proposition.

norbe

Sorry, but everyone uses that is not argument.

–1 for RFC. Open bug in your IDE instead.

Last edited by norbe (2014-11-05 09:51)

Jan Tvrdík

everyone uses that is not argument

That is certainly an argument, because we don't live in a bubble. It is not an ultimate argument (meaning we MUST do it because other people do it as well) but it is an argument.

But even if you decide to ignore the world around you (specification, real-world usage, IDE support), I still claim that it is a stronger binding and therefore less prune to have invalid types (as explained above)

Open bug in your IDE instead.

They have no reason to fix this (as explained above).

Last edited by Jan Tvrdík (2014-11-05 13:23)

looky

+1 for RFC, without alignment

David Grudl

@JanTvrdík you said only half of truth, only the “nicer” half of truth. What is really supported by the most popular PHP IDE and really used by top 10 PHP frameworks is this:

/**
 * Builds the view.
 * @param FormView          $view    The view
 * @param FormInterface     $form    The form
 * @param array             $options The options
 */
public function buildView(FormView $view, FormInterface $form, array $options)

or

/**
 * Builds an AcceptHeaderInstance instance from a string.
 * @param string $itemValue
 * @return AcceptHeaderItem
 */
public static function fromString($itemValue)

This is auto-generated garbage with no added value (every @param is useless). Generated and supported by the most popular PHP IDE and accepted by top 10 PHP frameworks, but still it is garbage. Adding garbage to Nette isn't something that could move development forward.

You know that writing parameter names in @param annotations in only the first step.

(Btw, who said „phpDoc should serve primary the user, not the phpDoc standard“?)

hrach

@DavidGrudl You are right that the structured doc is much more usable and important. So +1 for compulsory alignment! However, you are not true, that IDEs can't generate, eg. PhpStorm can. See the first checkbox:

Last edited by hrach (2014-11-05 14:30)

Jan Tvrdík

This is auto-generated garbage with no added value

But I don't propose to add any more @param annotation than what we already have. I propose to fix an existing problem.

who said „phpDoc should serve primary the user, not the phpDoc standard“

I did. Isn't that enough? I always thought that the experience of programmer using Nette is very important part of Nette development.

Last edited by Jan Tvrdík (2014-11-05 15:14)

David Grudl

If I understand you, this RFC was created because PhpStorm does not suggest type for parameters and also refused to implement it?

Generally, I am not against phpDoc cleanup, there are @param that should be removed, there are @param that can be removed because default value is enough, there are @params with meaningless description which can be removed.

But to modify phpDoc to work in the IDE with very buggy phpDoc support seems to me like step in the wrong direction. As you can see, writing parameter names in @param is not always solution:

hrach

@DavidGrudl

PHPStorm support is not the only one argument, there are others, more important (consistency with the rest, etc.)
This is not about phpdocing everything what could by phpdoced, so no phpdoc ⇒ not bad phpdoc. I agree with removing the redundant. (the first case)
There is big huge difference about string, or string|null. You example is buggy, same as many places in nette. phpdoc is just not correct.
Example of “can be removed” is not correct. Default valud should never imply accepted data type. Especially in php, where it's imposible to override methods (multiple definitions for different datatypes).
Examples of meaningless description: yes, it's not needed, just prefix it with $ and it will make much more benefit (valid with standard, consistency, code completion).
Last, not least, seems you don't know CTRL+Q:

hrach

But to modify phpDoc to work in the IDE

Also little but funny if I remember this commit: https://github.com/…e98b53843884. Honza propose compatibility with standard, not just with one particular IDE.

David Grudl

@hrach If the consistency with the rest had been an argument, Nette would follow PSR, ILogger in Tracy would implement Psr\Log\LoggerInterface etc… Thank God that it is not argument ;)

Why is my example incorrect?

hrach

Thank God that it is not argument.

I thinks it's pretty relevant argument. That's the reason why pepople use frameworks. The features are a secondary thing, the primary and most benefit is the, a little bit, standardized way how to write applications. It's ok if it is not the only rule, but it should be taken into consideration. Also, this standard is so much older and was involved progresively, not like psr, where one person created it from sratch. Moreover, this standard do not change the code, it's just soup around that, so I thinks is much more less pain to fit in it.

Your phpdoc is missing allowed null. (My screenshot is not full, I added null.)

Last edited by hrach (2014-11-05 18:00)

Jan Tvrdík

Thank God that it is not argument ;)

Once again – consistency is an argument, we don't live in a bubble. We live in an environment with other PHP projects and technologies in general. There would be very little benefit in supporting Composer (before Nette 2.2 ) if Nette was the only project in the world using it. It would be silly to use phpDoc if everyone else would you some other format. We are part of a community, we should play nice with each other.

That does not mean we always need to do things the same way as others, but we should consider their choices when we make our own.

Last edited by Jan Tvrdík (2014-11-05 18:16)

David Grudl

@hrach in your “correct” example I still see fnc(a : bool, [b = null]). Info about string is completely missing. So phpDoc support in PhpStorm seems crappy.

ad Default valud should never imply accepted data type (do you see bool?):

David Grudl

@JanTvrdík I understand you, but again: was this RFC created because PhpStorm does not suggest types? Or is there any other (not hypothetical) reason? Other not compatible IDE? Other not compatible tool?

Because Nette is compatible with phpDoc specification. PhpStorm is not. And it's phpDoc support seems to me buggy.

hrach

So phpDoc support in PhpStorm seems crappy.

agreed.

hrach

ad Default valud should never imply accepted data type (do you see bool?):

the sentence is given to author, not to the tool, it's absolutely correct it tries every possibility to resolve it.

David Grudl

I am asking for it over and over again, because I do not know what is the goal.

Do you want fit it to PhpStorm? Then I showed here that the writing parameter names in @param is not always the solution. (It seems that PhpStorm prefers syntax @param $a string and that's why @param string doesn't work – but it goes against the standard). In addition, the default value implies a type – if you want to fit it to PhpStorm, you should count with it.

Do you want to adapt the standard? Nette complies with it (unlike PhpStorm). Although there is a „not recommended notice“. The only problem is that phpDoc requires lowercased true, false, null.

Do you want to be consistent with other tool?

Jan Tvrdík

Do you want fit it to PhpStorm?

Yes, among other things. It is the most practical reason. However there are other reasons as mentioned above. I don't see it as making it work in PhpStorm but as making the type information more accessible to framework users.

writing parameter names in @param is not always the solution

I looked into this. PhpStorm resolves the type properly as null|string. It just does not render it in the tooltip. If case of null|int it is rendered fine. I don't see how some edge case bugs in PhpStorm are relevant here.

It seems that PhpStorm prefers syntax @param $a string

I don't think so. Why do you think that?

Do you want to adapt the standard?

Yes, if it is beneficial for framework users.

David Grudl

However there are other reasons as mentioned above.

Can you be more specific?

Jan Tvrdík

1) Interoperability now

Theory: Using something that is known to work and used by many is generally better for interoperability that using something that is known to not work well and used by nobody.

Real world: Current syntax does not work in PhpStorm, Eclipse and Komodo IDE (used also by Sublime Text, AFAIK). Proposed syntax works in all mentioned IDEs. There are probably even more tools where the situation is the same.

2) Interoperability in the future

Because the current syntax is used by nobody and officially not recommended, it may in the future stop being supported even by the tools which support it now. That is unlikely to happen to the proposed syntax.

3) Order independence consistency

All annotations in PHP are AFAIK order-independent except for @param without parameter name. Inconsistency is evil.

4) Cross language consistency

Both JavaDoc and JsDoc AFAIK require parameter name. Inconsistency is evil.

I don't think that any of those reasons ALONE is sufficient to make such aggressive change to Nette codebase. But they are not alone. If the current syntax was only PhpStorm's issue or if it was only not recommend or if it was only different from the world around us, I would not propose this RFC.

David Grudl

Ok, I will not prevent it, but in situations where function has only one parameter it is absolutely stupid.

bazo

Jan Tvrdík wrote:

Do you want fit it to PhpStorm?

Yes, among other things. It is the most practical reason. However there are other reasons as mentioned above. I don't see it as making it work in PhpStorm but as making the type information more accessible to framework users.

writing parameter names in @param is not always the solution

I looked into this. PhpStorm resolves the type properly as null|string. It just does not render it in the tooltip. If case of null|int it is rendered fine. I don't see how some edge case bugs in PhpStorm are relevant here.

It seems that PhpStorm prefers syntax @param $a string

I don't think so. Why do you think that?

Do you want to adapt the standard?

Yes, if it is beneficial for framework users.

phpStorm support is not a valid argument. if its phpDoc support is broken, the framework should not try to fix it

hrach

@DavidGrudl

Ok, I will not prevent it, but in situations where function has only one parameter it is absolutely stupid.

Of course it's stupid, but it's again about consistency. More important, about consistency in the particular project.

David Grudl

@hrach see this

The @var tag MUST contain the name of the element it documents. An exception to this is when property declarations only refer to a single property.

consistency…

Jan Tvrdík

@DavidGrudl Because you are the one making the decision, could you state you decision clearly? e.g. “Accepted”, “Accepted with the following notes…”, “Declined for the following reasons…”

David Grudl

Accepted with the following notes: it is big loss to change valid codebase to fit to editor, which do not respect standard, which is itself inconsistent (@var versus @param).