Implement rule from #131

[udovicic]

Implementation of rule from #131

[lenaorobei]

I run your branch against Magento codebase. Here are some suspicious findings:

FILE: app/code/Magento/Ui/Component/Wrapper/Block.php ------------------------------------------------------------------------------------------------------------------------------------------- FOUND 0 ERRORS AND 2 WARNINGS AFFECTING 2 LINES ------------------------------------------------------------------------------------------------------------------------------------------- 16 | WARNING | Motivation behind the added @deprecated tag MUST be explained. @see tag MUST be used with reference to new implementation. 18 | WARNING | Motivation behind the added @deprecated tag MUST be explained. @see tag MUST be used with reference to new implementation. ------------------------------------------------------------------------------------------------------------------------------------------- FILE: app/code/Magento/Authorizenet/Model/Directpost.php ------------------------------------------------------------------------------------------------------------------------------------------- FOUND 0 ERRORS AND 2 WARNINGS AFFECTING 2 LINES ------------------------------------------------------------------------------------------------------------------------------------------- 21 | WARNING | Motivation behind the added @deprecated tag MUST be explained. @see tag MUST be used with reference to new implementation. 23 | WARNING | Motivation behind the added @deprecated tag MUST be explained. @see tag MUST be used with reference to new implementation. ------------------------------------------------------------------------------------------------------------------------------------------- 

[udovicic]

Hi @lenaorobei,

Thanks for pointing that out, it appears that part of code responsible for detecting DocBlock of constants has a bug, and instead detecting that there is no const DocBlock, it detects one belonging to a class. In both outlined examples, there is a problem with DocBlock as pointed out, but only for classes. Which means that this change is actually ok, problem is one of previous commits. But since is mine either way, I will fix it and report back.

[lenaorobei]

@udovicic thank you for the fix.

I'm just thinking out loud here. The rule says:

@see tag MUST be used with reference to new implementation when code is deprecated and there is a new alternative.

And what if there is no alternative? Example:

/**  * Authorize.net Payment CC Types Source Model  * @deprecated 2.3.1 Authorize.net is removing all support for this payment method  */ class Cctype extends PaymentCctype

Does it make sense to add check: if explanation is provided after the @deprecated tag, @see is not required, but if @deprecated is not followed by any description, @see must be present. Ideal world is take version into account.

• @deprecated 2.3.1 ❌
• @deprecated 2.3.1 Authorize.net is removing all support for this payment method ✅

Need your thoughts here. We don't want to bother developers with false positive warnings.

[udovicic]

Opens up a room for being lazy and not specifying the alternative. On the other hand, it can't stay the way it is. So I agree with your proposal. Do you want me to change the source or do you want to consult with someone first?

[lenaorobei]

@udovicic let me discuss with internal teams and I'll get back to you.

[lenaorobei]

Let's add additional check if explanation is provided after the @deprecated tag, @see is not required, but if @deprecated is not followed by any description, @see must be present. Let's also do not take version into account and assume it is a description. Thanks.

[udovicic]

Hi @lenaorobei , just to make sure I fully understand last part, version is not counted as description? So examples that you provided:

@deprecated 2.3.1 ❌
@deprecated 2.3.1 Authorize.net is removing all support for this payment method ✅

Still stand correct?

[lenaorobei]

Sorry, I should have explain better. For now both cases should be fine.
@deprecated 2.3.1 ✅
@deprecated 2.3.1 Authorize.net is removing all support for this payment method ✅

[udovicic]

@lenaorobei I have modified code to reflect latest request. I am not particularly happy with how it looks, but I have no idea how to refactor it to look better either.