Skip to content
This repository has been archived by the owner on Feb 20, 2019. It is now read-only.

Add guideline about annotation using one-line #657

Closed
wants to merge 2 commits into from

Conversation

LukasReschke
Copy link
Contributor

From owncloud/core#12279 (comment)

Opinions:

@LukasReschke
Copy link
Contributor Author

Summoning some other core devs:

@icewind1991 @schiesbn @butonic @georgehrke @blizzz @tomneedham @bantu @PVince81 @MorrisJobke @th3fallen (Hope I missed no-one that might be interested as well!)

Reason for that is that currently some annotate the type using three-line comments, others one-liners and then again others don't do it at all.

For the sake of Vertical Density I'd suggest that we use single line comments instead of bloating too much comments into it.

@LukasReschke
Copy link
Contributor Author

Please notice that PHPStorm can generate such member variables annotations on it's own just by typing /** one line above the variable.

@icewind1991
Copy link
Contributor

My preference goes out to the three line variant.

Additionally a note about using the fully qualified name versus just the classname would be useful (\OC\Foo\Bar vs Bar), phpstorm defaults to the later I prefer the former

@MorrisJobke
Copy link
Contributor

One line full qualified nane

@butonic
Copy link
Contributor

butonic commented Nov 19, 2014

My vote is one line, class name only.

The full qualified type is resolved via the use statement, no need to duplicate.

PSR5 states that @var is deprecated for properties and @type should be used instead. phpStorm 8.0+ can be configured to use one line as well as full-qualified types: https://www.jetbrains.com/phpstorm/webhelp/creating-php-documentation-comments.html

Method body comments for variables still use @vareg :

    /**
     * @param Node[] $nodes
     * @param \OC_EventSource $eventSource
     */
    public function doSth (array $nodes, \OC_EventSource $eventSource = null) {
        foreach ($nodes as $node) {
            $node->...      // codehinting works
        }
        /* or */
        if (isset($nodes[0])) {         
            /** @var File */
            $file = $nodes[0];
            $file->...          // codehinting works
        }
        /* ... */
    }

@blizzz
Copy link
Contributor

blizzz commented Nov 19, 2014

/me votes for one-line and relative name

@icewind1991
Copy link
Contributor

My argument for FQN is that it provides the full information in one place, with relative names I need to look at both the current namespace and use statements before I know what class is being referenced

@nickvergessen
Copy link
Contributor

1line, dont care about full/use as long as the use is present and therefor the methods are found

@BernhardPosselt
Copy link
Contributor

Same opinion as @butonic

@DeepDiver1975
Copy link
Contributor

I honestly don't care - as long as my IDE can handle it 🙈

Generally speaking: following PSR is desired.

@MorrisJobke
Copy link
Contributor

If there is a PSR for that -> use that

* @param \OC_Defaults $defaults
* @param \OCP\IRequest $request
*/
public function __construct(\OC_Defaults $defaults, \OCP\IRequest $request) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably bad idea to use a private namespace class

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

woot? This is just to point out how doc blocks should look like

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep ;)

@BernhardPosselt
Copy link
Contributor

👍

@carlaschroder
Copy link
Contributor

@LukasReschke Is this ready to merge?

@jospoortvliet
Copy link
Contributor

@LukasReschke ping?

@carlaschroder
Copy link
Contributor

@LukasReschke Any reason to keep this open?

@LukasReschke LukasReschke deleted the add-annotations-single branch October 22, 2015 15:00
@MorrisJobke
Copy link
Contributor

@LukasReschke Can we just merge it with the use-statement?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

10 participants