Skip to content
This repository has been archived by the owner on Jan 1, 2023. It is now read-only.

PhpDocs update #127

Open
wants to merge 10 commits into
base: dev
Choose a base branch
from
Open

PhpDocs update #127

wants to merge 10 commits into from

Conversation

xtrime-ru
Copy link
Contributor

@xtrime-ru xtrime-ru commented Jan 26, 2021

  1. Fixed style of generated phpDocs to allow IDE use them.
  2. Replaced callbacks with JsHandler
  3. Detailed arguments in traits.

Tested in PHPStorm.

P.S. Docs generation from js code is awesome yet simple to read!

@xtrime-ru
Copy link
Contributor Author

Hello!

Can anybody check this PR and merge or send feedback?

@nesk
Copy link
Member

nesk commented Feb 9, 2021

Hi! I've started to write a review, I will try to finish it tonight.

@xtrime-ru
Copy link
Contributor Author

Many thanks! There is no hurry. I just wanted to know, if i need to wait little bit more or this PR will be buried in sands of time :)

@xtrime-ru
Copy link
Contributor Author

xtrime-ru commented Feb 9, 2021

To make review simpler, here is the list of files i have changed:

  • composer.json
  • src/Command/GenerateDocumentationCommand.php
  • src/Traits/AliasesEvaluationMethods.php
  • src/Traits/AliasesSelectionMethods.php
  • src/doc-generator.ts

After that i run docs generator cli command to update automatically generated phpDocs.

composer.json Outdated Show resolved Hide resolved
src/Puppeteer.php Outdated Show resolved Hide resolved
src/Resources/Browser.php Outdated Show resolved Hide resolved
src/Resources/Browser.php Outdated Show resolved Hide resolved
src/Resources/Coverage.php Outdated Show resolved Hide resolved
src/Traits/AliasesSelectionMethods.php Outdated Show resolved Hide resolved
@nesk
Copy link
Member

nesk commented Feb 9, 2021

P.S. Docs generation from js code is awesome yet simple to read!

I'm glad it helps you!

@xtrime-ru xtrime-ru changed the title PhpDocs update WIP: PhpDocs update Feb 10, 2021
@xtrime-ru xtrime-ru changed the title WIP: PhpDocs update PhpDocs update Feb 17, 2021
composer.json Outdated Show resolved Hide resolved
@xtrime-ru xtrime-ru requested a review from nesk February 17, 2021 14:56
@xtrime-ru
Copy link
Contributor Author

@nesk Ive reverted Traits changes.
But i still think we need to fix them :) :
image

Also i've done some stuff:

  1. Add Puppeteer methods from Puppeteer and PuppeteerNode
  2. Remove build folder before building docs
  3. Added JsFunction type instead of callbacks. Also added replacement of mixed to JsFunction for evaluation* methods.

Will wait for your approval and merge ;)

@nesk
Copy link
Member

nesk commented Feb 18, 2021

IDE autocompletion issues

I have downloaded PHPStorm and tried to setup everything, I must agree: it doesn't work well. 😅

However, you can clearly see with this PHPStan demo what it brings to have those annotations, they really help. I'm don't want to give up on them, but I have some ideas to fix the current issues.

Here are some interesting points:

The good news:

  • PHPStan can use @phpstan-method instead of @method, which would allow to fully type the methods without breaking Psalm and IDEs.
  • Psalm can use @psalm-method instead of @method, which would allow to use better typings than the ones for IDEs and not break on PHPStan format.
  • In addition, we can add simple @method annotations which are working everywhere and will allow IDEs to have proper autocompletion.

By using the three documentation formats here, we could achieve perfect support everywhere.

Callback documentation

You are right, the current typings for the callbacks aren't the good ones, because PuPHPeteer expects you to use a JsFunction instance. You've made the following changes:

- * @method mixed download(string $revision, callable(float $x, float $y): void $progressCallback = null)
+ * @method mixed download(string $revision, \Nesk\Rialto\Data\JsFunction $progressCallback = null)

This is correct, however we lose the detailed typings. I think we can fix this by doing this:

- * @method mixed download(string $revision, callable(float $x, float $y): void $progressCallback = null)
+ * @method mixed download(string $revision, (callable(float, float): void)|JsFunction $progressCallback = null)

This works perfectly and can still help users to see the detailed typings, here's an example.

Additionally, I could add in Rialto a trigger to warn users about the fact they are using a callable instead of a JsFunction instance.

FQCN references

What do you think? Do you insist on full namespaces instead of FQCN?

I've tought about it, I see no difference on your side (as a user) if I use FQCN refs or relative ones. As the maintainer of the project, I would rather keep the FQCN references since I find them more reliable.

PHPStorm makes no difference between relative or FQCN references, it always displays the FQCN one:

Capture d’écran 2021-02-18 à 13 36 03

Capture d’écran 2021-02-18 à 13 36 15

To conclude

I see immediate benefits in some of your changes:

  • You have fixed the missing symfony/console dependency. This is already merged in another PR, thank you again!
  • You have fixed the missing $ character in src/Traits/AliasesEvaluationMethods.php.
  • You have fixed the missing properties and methods in src/Puppeteer.php.

If you want, you can make a new PR with only the last two changes in this list and I will merge it right away. 🙂

About everything else, there is still work to do:

  • The documentation generator should output three formats for each property/method/etc to support the majority of the tools, namely PHPStan, Psalm and all other tools reading phpDoc (like IDEs).
  • The documentation for callbacks should support JsFunction instances without removing the detailed typings, a union can solve this.

If you don't have time anymore to work on these, I can take over. However, I may not have time to work on this soon so you will have to deal with the current typings for a while.

@xtrime-ru
Copy link
Contributor Author

  1. IDE autocompletion issues

Good idea to use @method, @phpstan-method, @psalm-method. I will modify doc-genereator. Add some property, to contriol output format of docs. We will run it 3 times, with different formats, and then merge them and write to phpDocs.

  1. Callback documentation

Union is ok, i will do that.

  1. FQCN references

Ok. Will return full namespace

Thanks for nice ideas, will implement them in upcoming days.

@nesk nesk marked this pull request as draft February 19, 2021 12:26
@xtrime-ru
Copy link
Contributor Author

xtrime-ru commented Apr 14, 2021

Hi! Sorry for delay with this fixes.

Fixed: FQCN references and callbacks unions (for phpstan format).
Added support for multiple doc styles for php.

But I ve run into another issue: phpStorm don't support @phpstan-method with our annotations. And it complains about duplicate methods:
image

also hints are incorect:
image

My proposal: replace @phpstan-method with non annotation string, like @method-extended. This is only way i can manage phpStorm works:
image

@xtrime-ru xtrime-ru marked this pull request as ready for review April 14, 2021 17:26
@xtrime-ru
Copy link
Contributor Author

@nesk Hi! Want to remind about my pull request :)

@xtrime-ru
Copy link
Contributor Author

Hi! PhpStorm got update with generics support in PHPDocs. Maybe this will help to cleanup this PR.

@xtrime-ru
Copy link
Contributor Author

Hi! PhpStorm got update with generics support in PHPDocs. Maybe this will help to cleanup this PR.

Checked in new PhpStorm and there is still no support for array shape for argement types in @method. So PR will be same.

Arrow functions fix & #private tags
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.

2 participants