Skip to content
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

#162 Modified @deprecated validator to consider it valid if comment is absent but @see tag is set #181

Merged
merged 8 commits into from
Sep 2, 2020

Conversation

sinisa86
Copy link
Contributor

@sinisa86 sinisa86 commented Apr 4, 2020

Modified @deprecated validator to consider it valid if a comment is absent but @see tag is set.
see #162

@m2-community-project
Copy link

@sinisa86 unfortunately, only members of the maintainers team are allowed to assign developers to the pull request

@m2-community-project
Copy link

@sinisa86 unfortunately, only members of the maintainers team are allowed to assign developers to the pull request

return true;
if ($seeTagRequired) {
return false;
} else {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

else can be eliminated here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we remove "else" branch there, it could lead to unexpected behavior because of the next IF condition:
if ($tokens[$seePtr + 2]['code'] !== T_DOC_COMMENT_STRING) { return false; }
That happens because $seePtr could be -1, so $seePtr + 2 points to how-knows-what.
"Else" part is required because it covers behavior when @see is not present at all.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, it really fails on some regular tests when I remove "else" branch.

Copy link
Contributor

@lenaorobei lenaorobei Apr 22, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

$seeTagRequired is boolean and if it is false, execution will drop to else or directly to the return.

I propose to simplify code even more:

        $seePtr = $this->getTagPosition('@see', $commentStartPtr, $tokens);
        if ($seePtr === -1) {
            return !$seeTagRequired;
        }
        return $tokens[$seePtr + 2]['code'] === T_DOC_COMMENT_STRING;

Copy link
Contributor

@lenaorobei lenaorobei left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sinisa86 could you please address review comments?

@sinisa86
Copy link
Contributor Author

@sinisa86 could you please address review comments?

Done.
I'm still a little bit more for an option before applying the last changes because it gives you information about whether @deprecated is well-formatted (true/false). Now, when it returns !seeTagRequired, it doesn't tell clearly what's returned in that case (it returns the information that seeTagIsRequired or not, which may seem unrelated to the function name and its purpose). Although this provides the same functionality, it doesn't provide the same level of clarity.
On the bottom line - it's a just one "IF" condition :)

@ihor-sviziev
Copy link
Collaborator

@lenaorobei could you check this PR?

ihor-sviziev
ihor-sviziev previously approved these changes Aug 31, 2020
@lenaorobei
Copy link
Contributor

@sinisa86 thanks for this contribution and your patience!

@lenaorobei lenaorobei merged commit 7f6fd20 into magento:develop Sep 2, 2020
magento-devops-reposync-svc pushed a commit that referenced this pull request May 31, 2022
…sage

AC-3187: Incorrect message for discouraged functions
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants