puphpeteer icon indicating copy to clipboard operation
puphpeteer copied to clipboard

PhpDocs update

Open xtrime-ru opened this issue 4 years ago • 12 comments

  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 avatar Jan 26 '21 16:01 xtrime-ru

Hello!

Can anybody check this PR and merge or send feedback?

xtrime-ru avatar Feb 09 '21 12:02 xtrime-ru

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

nesk avatar Feb 09 '21 12:02 nesk

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 avatar Feb 09 '21 13:02 xtrime-ru

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.

xtrime-ru avatar Feb 09 '21 13:02 xtrime-ru

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

I'm glad it helps you!

nesk avatar Feb 09 '21 18:02 nesk

@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 ;)

xtrime-ru avatar Feb 17 '21 15:02 xtrime-ru

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.

nesk avatar Feb 18 '21 13:02 nesk

  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.

xtrime-ru avatar Feb 19 '21 12:02 xtrime-ru

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 avatar Apr 14 '21 17:04 xtrime-ru

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

xtrime-ru avatar Apr 27 '21 14:04 xtrime-ru

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

xtrime-ru avatar Jul 30 '21 10:07 xtrime-ru

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.

xtrime-ru avatar Aug 11 '21 15:08 xtrime-ru