Skip to content

#58 : Implement sniff for class properties PHPDoc formatting #140

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 16 commits into from
Sep 7, 2021
Merged

#58 : Implement sniff for class properties PHPDoc formatting #140

merged 16 commits into from
Sep 7, 2021

Conversation

konarshankar07
Copy link
Contributor

This PR added new rule for class properties for PHP Doc block
Fixed #58

@lenaorobei
Copy link
Contributor

Looking at failed Travis and see that sniff produces false positive findings, for example it should not be triggered for method declaration.

@lenaorobei lenaorobei added the needs update PR or issue needs update from the author label Sep 10, 2019
@konarshankar07
Copy link
Contributor Author

Hello @lenaorobei ,
I'm looking into this issue why its validating method declaration and I'll update you soon.
Thanks

@lenaorobei
Copy link
Contributor

lenaorobei commented Sep 12, 2019 •
edited
Loading

@konarshankar07 please cover also cases with meaningless description. Examples:

    /**
     * @var Test
     *
     * Product Repository
     */
    private $productRepository;

Method for this check already exists https://github.com/magento/magento-coding-standard/blob/develop/Magento2/Sniffs/Commenting/PHPDocFormattingValidator.php#L58

lenaorobei
lenaorobei reviewed Sep 12, 2019
View reviewed changes
Magento2/Tests/Commenting/ClassPropertyPHPDocFormattingUnitTest.php Outdated
public function getWarningList()
{
return [
12 => 1,
Copy link
Contributor

Choose a reason for hiding this comment

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

It should detect on line 13.

lenaorobei
lenaorobei reviewed Sep 12, 2019
View reviewed changes
Magento2/Tests/Commenting/ClassPropertyPHPDocFormattingUnitTest.inc
private $_multipleClassAttribute = '';

private $_missingDocBlockClassAttribute = '';
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add here positive scenarios as well. Some examples with correctly formatted blocks.

@konarshankar07
Copy link
Contributor Author

@konarshankar07 please cover also cases with meaningless description. Examples:

/**
     * @var Test
     *
     * Product Repository
     */
    private $productRepository;

Method for this check already exists https://github.com/magento/magento-coding-standard/blob/develop/Magento2/Sniffs/Commenting/PHPDocFormattingValidator.php#L58

Hello @lenaorobei ,
The mentioned method is used to check if the Doc block has any meaningful short description or not. As per you example, We need to check the formatting of PHP doc block. Correct me If I'm wrong.
Thanks

@lenaorobei
Copy link
Contributor

@konarshankar07, yes. The idea behind this is that $productRepository variable already has meaningful name, so short description with Product Repository should raise a warning.

@konarshankar07
Copy link
Contributor Author

Hello @lenaorobei ,
Can I get an update for this PR?
Thanks

@lenaorobei
Copy link
Contributor

Hi @konarshankar07, sure. This sniff is not just about formatting. As per our conversation, please use PHPDocFormattingValidator to check the description and add these cases to the unit test.

@konarshankar07
Copy link
Contributor Author

Hello @lenaorobei ,
Can I get any update here?
Thanks

@lenaorobei
Copy link
Contributor

@konarshankar07 I will test it against Magento codebase and will get back to you.

lenaorobei
lenaorobei requested changes Oct 30, 2019
View reviewed changes
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.

I run against Magento 2 codebase and discovered some false positives:

    /**
     * @var \Magento\Framework\View\Element\UiComponent\DataProvider\DataProviderInterface|
     *      \PHPUnit_Framework_MockObject_MockObject
     */
    private $dataProviderMock;

Expected: ✅ Actual: ❌

In \Magento\Ui\Component\MassAction\Columns\Column issue detected on line 31, but should be on 22 and 29.

File \Magento\Eav\Model\AttributeDataFactory caused An error occurred during processing; checking has been aborted. The error message was: Undefined index: comment_opener in magento-coding-standard/Magento2/Sniffs/Commenting/ClassPropertyPHPDocFormattingSniff.php on | | line 65

    /**
     * @var ArrayUtils|\PHPUnit_Framework_MockObject_MockObject
     */
    protected $arrayUtils;

Expected: ✅ Actual: ❌

Please address found issues and see some suggestions.

@konarshankar07
Copy link
Contributor Author

konarshankar07 commented Oct 31, 2019 •
edited
Loading

Thanks for the feedback

@sivaschenko
Merge branch 'develop' of github.com:magento-commerce/magento-coding-…
7e83297 
…standard into implement-sniff-for-class-properties--task-58
@sivaschenko
Copy link
Member

@konarshankar07 thank you for the pull request! Would you be able to resolve the test failures?

@konarshankar07
Copy link
Contributor Author

Hello @sivaschenko
Sure, I will work on the test failure
Thanks

@konarshankar07
Copy link
Contributor Author

@sivaschenko ready for review

svera
svera reviewed Sep 7, 2021
View reviewed changes
Magento2/Sniffs/Commenting/ClassPropertyPHPDocFormattingSniff.php
return;
}
$error = 'Short description duplicates class property name.';
$phpcsFile->addWarning($error, $isShortDescriptionAfterVar, 'AlreadyHaveMeaningFulNameVar');
Copy link
Contributor

Choose a reason for hiding this comment

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

Correct camel casing: AlreadyHaveMeaningfulNameVar

Magento2/Sniffs/Commenting/ClassPropertyPHPDocFormattingSniff.php
return;
}
$error = 'Short description duplicates class property name.';
$phpcsFile->addWarning($error, $isShortDescriptionPreviousVar, 'AlreadyHaveMeaningFulNameVar');
Copy link
Contributor

Choose a reason for hiding this comment

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

Correct camel casing: AlreadyHaveMeaningfulNameVar

Magento2/Sniffs/Commenting/ClassPropertyPHPDocFormattingSniff.php
null,
false
);
if ($this->PHPDocFormattingValidator->providesMeaning(
Copy link
Contributor

Choose a reason for hiding this comment

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

This if block can be extracted into a separate method, as it basically repeats is the same as the check done for checking description before @var tag. This way, we reduce repetition and also function size, as processMemeberSize() is quite big, helping readability along the way

@svera
Copy link
Contributor

svera commented Sep 7, 2021

@magento import pr to magento-commerce/magento-coding-standard

@magento-engcom-team
Copy link
Contributor

@svera the pull request successfully imported.

@magento-devops-reposync-svc magento-devops-reposync-svc merged commit 3bd8317 into magento:develop Sep 7, 2021
@svera svera mentioned this pull request Sep 13, 2021
Merged
magento-devops-reposync-svc pushed a commit that referenced this pull request Dec 20, 2021
@svera
Merge pull request #140 from magento-trigger/AC-99
6f2a73b 
AC-99: Update Third Party library: jquery.tabs
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@svera svera svera left review comments

@lenaorobei lenaorobei Awaiting requested review from lenaorobei

Assignees

@svera svera

@lenaorobei lenaorobei

Labels
needs update PR or issue needs update from the author Progress: done
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[New Rule] Implement sniff for class properties PHPDoc formatting
7 participants
@konarshankar07 @lenaorobei @sivaschenko @svera @magento-engcom-team @magento-devops-reposync-svc @diazwatson