Calling Developers!
We are reenergizing our code contribution process! Learn More

What are the Slack Archives?

It’s a history of our time together in the Slack Community! There’s a ton of knowledge in here, so feel free to search through the archives for a possible answer to your question.

Because this space is not active, you won’t be able to create a new post or comment here. If you have a question or want to start a discussion about something, head over to our categories and pick one to post in! You can always refer back to a post from Slack Archives if needed; just copy the link to use it as a reference..

When running the code style sniffer upon commit, I'm getting errors like this: ```FOUND 1 ERROR AFFE

Options
U01LKKBK97T
U01LKKBK97T Posts: 287 🧑🏻‍🚀 - Cadet

When running the code style sniffer upon commit, I'm getting errors like this:

FOUND 1 ERROR AFFECTING 1 LINE
----------------------------------------------------------------------
 173 | ERROR | [x] @throw annotation `\Exception` superfluous and
     |       |     needs to be removed
----------------------------------------------------------------------

Indeed, my code might throw an exception at this very place. Thus, having a @throws annotation in the function's docblock should be fully correct. How would the calling function know that it needs to catch it otherwise? Am I missing something here or is the code sniffer behaving wrongly here?

Comments

  • U013EK8RD7A
    U013EK8RD7A Posts: 127 🧑🏻‍🚀 - Cadet
    Options

    Just a guess, but perhaps it’s because You’re using a generic Exception class. If You’d throw “SomeSpecificException” exception this error would go away.

  • U01LKKBK97T
    U01LKKBK97T Posts: 287 🧑🏻‍🚀 - Cadet
    Options

    Just wanted to give you a most basic example.
    It also appears with custom exceptions, unfortunately.

  • U01LKKBK97T
    U01LKKBK97T Posts: 287 🧑🏻‍🚀 - Cadet
    Options

    Interesting enough that the sniffer complains about the @throw annotation while I'm using @throws, actually. Would that make the difference maybe?

  • Unknown
    edited October 2021
    Options

    Yes, needs to be throws as per standards afaik.

  • U01LKKBK97T
    U01LKKBK97T Posts: 287 🧑🏻‍🚀 - Cadet
    Options

    My assumption is that the sniffer will complain if the annotated function doesn't explicitly throw this type of exception. If e. g. it is thrown in a nested function, it won't recognize it.

  • vasily.rodin
    vasily.rodin Freelance Spryker Developer Spryker Solution Partner Posts: 25 ✨ - Novice
    Options

    As far as I remember, this code sniffer rule checks only exceptions, which you explicitly define inside the method body. So if exception is not directly in the method body, annotation is not needed

  • U01LKKBK97T
    U01LKKBK97T Posts: 287 🧑🏻‍🚀 - Cadet
    Options

    Yes, looks like it. But how would you get around this then? Catching and rethrowing the exception as a workaround doesn't feel right.

  • vasily.rodin
    vasily.rodin Freelance Spryker Developer Spryker Solution Partner Posts: 25 ✨ - Novice
    Options

    https://github.com/spryker/code-sniffer/blob/07022098a526f7d64a04da148c95d86e93360111/Spryker/Sniffs/Commenting/DocBlockThrowsSniff.php#L18
    Looks like it’s intentional:

    We only ever declare them for the exceptions inside the own method.
    

    So you can extend this sniff, or disable it. Or maybe @UQK3ZPJEN can suggest better, since he’s author of this sniff

  • yes, we only use primary level exceptions, as nested starts to lie soon (as it is almost impossible to assert by sniffer).

  • you can always remove this on project level - or replace it.

  • What is the concrete code in your method?

  • U01LKKBK97T
    U01LKKBK97T Posts: 287 🧑🏻‍🚀 - Cadet
    Options

    It's basically this:

    /**
     * @throws \Exception
     */
    protected function foo()
    {
        // this will throw an exception that should be propagated to the caller of foo()
        $this->nestedFoo();
    }
    
  • In that case you could only extend/overwrite/silence the core sniffer rule.
    I wonder if a bang-overwrite could make sense

    * @throws \Exception !
    

    with the ! as comment it could ignore the "removal" requirement.

    I use a similar approach for IDE helper based annotations to be auto-managed.

  • U01LKKBK97T
    U01LKKBK97T Posts: 287 🧑🏻‍🚀 - Cadet
    Options

    I was hoping for something like that. But doesn't help, unfortunately.

  • U01LKKBK97T
    U01LKKBK97T Posts: 287 🧑🏻‍🚀 - Cadet
    Options
    /**
     * @param array $tourTime
     * @param string $localeTransfer
     *
     * @throws \Exception !
     *
     * @return string
     */
    protected function getDeliveryDateText(array $tourTime, string $localeTransfer): string
    {
        ...
    }
    

    still gives

    ----------------------------------------------------------------------
    FOUND 1 ERROR AFFECTING 1 LINE
    ----------------------------------------------------------------------
     174 | ERROR | [x] @throw annotation `\Exception` superfluous and
         |       |     needs to be removed
    ----------------------------------------------------------------------
    
  • "wonder" + "could make sense" => I was only spitballing ideas here, none of this is of course implemented.

  • U01LKKBK97T
    U01LKKBK97T Posts: 287 🧑🏻‍🚀 - Cadet
    Options

    Sure, I appreciate any idea. Didn't expect it to be final, but hey, we could have been lucky.

  • I added the idea here for further feedback from everyone: https://github.com/spryker/code-sniffer/issues/275
    Also, if someone wants, they can do a PR here to support this.

  • @U01LKKBK97T Can you check if https://github.com/spryker/code-sniffer/pull/278 would work here as expected?
    I am planning a release for end of this week, so this fit in with almost no work needed.

  • U01LKKBK97T
    U01LKKBK97T Posts: 287 🧑🏻‍🚀 - Cadet
    Options

    Seems to work, great job!