Skip to content

Annotations refactoring #710

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 17 commits into from
Jul 12, 2020
Merged

Annotations refactoring #710

merged 17 commits into from
Jul 12, 2020

Conversation

Vincz
Copy link
Collaborator

@Vincz Vincz commented Jul 11, 2020

Q A
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? yes
Documented? not yet
Fixed tickets #...
License MIT

Vincz added 4 commits July 11, 2020 14:54
- Annotations Refactoring (with a GraphClass)
- Add missing `isTypeOf` on `@GQL\Type`
- Add `targetType` on `@GQL\Mutation` to handle multiple schema #526
- Autodiscover Interfaces for '@gql\Type' based on PHP interfaces or parent classes #701
- Autodiscover Union types for '@gql\Union` based on PHP interfaces or children classes #701
- Annotations Refactoring (with a GraphClass)
- Add missing `isTypeOf` on `@GQL\Type`
- Add `targetType` on `@GQL\Mutation` to handle multiple schema #526
- Autodiscover Interfaces for '@gql\Type' based on PHP interfaces or parent classes #701
- Autodiscover Union types for '@gql\Union` based on PHP interfaces or children classes #701
@Vincz Vincz changed the title [WIP] Annotations refactoring Annotations refactoring Jul 12, 2020
@Vincz Vincz requested review from murtukov and mcg-web July 12, 2020 09:24
Fix my bad english. Thanks @murtukov

Co-authored-by: Timur Murtukov <[email protected]>
@murtukov
Copy link
Contributor

murtukov commented Jul 12, 2020

@Vincz LGTM, just only one general thing: try to always specify the types in docblocks (param and return) and inline. If you look at the badges from branch 0.11 to master, you will see, that the Scrutinizer reduces with every new version, which means the code quality drops.

For example, if the method returns array, add to docblock something like:

  • array<int, MyClass> - meaning that all keys are integers and all items are objects
  • array<MyClass|YourClass> - meaning that array contains objects either of class MyClass or YourClass
  • array<int, MyClass|YourClass> - same as above, but more specific about keys.

When you specify multiple types like MyClass|YourClass, it can create another errors, saying that some methods don't exist. In this case just add inline type with /** @var <CLASSNAME> $var */ comment.

And try to fix the errors reported from Scrutinizer. Right now Scrutinizer says, that the 2 worst classes in the whole project are: TypeBuilder and AnnotationParser. It says, that these files are too big and are too complex and that's true. I am planning to split the TypeBuilder (because I created it) into multiple more simple classes and would suggest you to do the same with AnnotationParser if that's possible.

@Vincz
Copy link
Collaborator Author

Vincz commented Jul 12, 2020

Thanks for the great review @murtukov !
I'm not sure how to handle the /** @var <CLASSNAME> $var */ you suggested when it can have multiple classes. Do you mind to give me an example for this one ? F
I agree about the file size, I already tried to reduced it a bit with the introduction of the GraphClass but it's not enough. I'll try to split it also.

@Vincz
Copy link
Collaborator Author

Vincz commented Jul 12, 2020

For example, with something like this :

if (!$fieldType) {
                if ($isMethod) {
                    if ($reflector->hasReturnType()) {

I know that because $isMethod is true, my $reflector is an instance of ReflectorMethod. How to make it understand ?

@murtukov
Copy link
Contributor

@Vincz

Example:

/**
 * @param FirstClass|SecondClass $myObject
 */
public function myMethod(object $myObject) 
{
    if($this->type === 'test') {
        $result = $myObject->doSomething();
        return $result;
    }

    $result = $myObject->doAnotherThing();
    return $result;
}

Here we say, that $myObject can be either of class FirstClass or SecondClass. Let's say the doSomething exists only in FirstClass and the doAnotherThing exists only in SecondClass.

In this case static analyzer will raise an error, saying, that doSomething doesn't exist in SecondClass, because it doesn't know in which conditions the method myMethod will be called. You can fix this by addind 2 comments:

/**
 * @param FirstClass|SecondClass $myObject
 */
public function myMethod(object $myObject) 
{
    if($this->type === 'test') {
        /** @var FirstClass $result */
        $result = $myObject->doSomething();
        return $result;
    }

    /** @var SecondClass $result */
    $result = $myObject->doAnotherThing();
    return $result;
}

With that, static analyzers know for sure, what class is used in what line.

@murtukov
Copy link
Contributor

@Vincz But this errors maybe wouldn't happend, if we defined param types and return types on all methods we use in the project. Static analyzer can then calculate all possible calls and determine itself, which types are where used.

Here is a useful link: https://phpstan.org/writing-php-code/phpdoc-types

@Vincz
Copy link
Collaborator Author

Vincz commented Jul 12, 2020

Ok, thanks for the explanation ! Is there a way to let it know the other way around. Like "It is not a ".
For example here :

if (null !== $gqlType) {
    if (!$gqlName) {
        $gqlName = $classAnnotation->name ?: $graphClass->getShortName();
    }

And need to let him know that it is not a @GQL\Provider (the only one without ->name).
Or should I explicitly indicate all the possible types ? /** @var First|Second|Third|... $classAnnotation **/

@murtukov
Copy link
Contributor

murtukov commented Jul 12, 2020

@Vincz you can create an interface NamedAnnotationInterface and implement all your annotations with the name property from it. Then you can add /** @var NamedAnnotationInterface $classAnnotation */. The name of course is on you, what makes more sense to group all these annotations together.

Update:
Actually, if you created an interface, you dont have to add a comment. You can simply add NamedAnotationInterface as type-hint instead of just object

Vincz and others added 5 commits July 12, 2020 12:44
# Conflicts:
#	src/Config/Parser/AnnotationParser.php
…xists

Simplify FQCN when using class_exists
- Annotations Refactoring (with a GraphClass)
- Add missing `isTypeOf` on `@GQL\Type`
- Add `targetType` on `@GQL\Mutation` to handle multiple schema #526
- Autodiscover Interfaces for '@gql\Type' based on PHP interfaces or parent classes #701
- Autodiscover Union types for '@gql\Union` based on PHP interfaces or children classes #701
@Vincz Vincz requested a review from murtukov July 12, 2020 14:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants