Skip to content

Implement rule from #107 #116

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 2 commits into from
Jul 17, 2019

Conversation

udovicic
Copy link
Member

In compliance with devdocs, rule will check if all occurrences of define() and const have dock block in front of them.

As #107 suggest, it will emit warning with severity 5.

@magento-cicd2
Copy link

magento-cicd2 commented Jun 23, 2019

CLA assistant check
All committers have signed the CLA.

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.

@udovicic thank you for the contribution, it will be great improvement! I have few comments:

  1. Please use strict comparison.
  2. The rule says:

Constants must have short description if they add information beyond what the constant name supplies.

Suggest adding check for comments that do not add additional info.

Should raise the warning:

/**
 * Nesting separator.
 */
const NESTING_SEPARATOR = '->';

/**
 * 
 */
const NUMBER_TWO = 2;

Same for define.

Should not raise the warning:

const NESTING_SEPARATOR = '->';

/**
 * Some great explanation here.
 */
const NUMBER_TWO = 2;

@udovicic
Copy link
Member Author

Hi @lenaorobei ,

Suggestions about strict checks are accepted, let me know if you want them squashed.

About the part:

if they add information beyond what the constant name supplies.

That is not what the cited devdocs say. There it is defined as implemented in this rule:

Constants must have short description.

@lenaorobei
Copy link
Contributor

Here is the open PR to the Magento DevDocs magento/devdocs#4425 in order to change this statement.

@udovicic
Copy link
Member Author

Makes sense, I will update as soon as I can.

@lenaorobei
Copy link
Contributor

One more thing I didn't notice yesterday. Could you please move this rule to the different namespace, for example Commenting or DocBlock instead of PHP. Thank you!

@lenaorobei lenaorobei added the needs update PR or issue needs update from the author label Jun 28, 2019
@udovicic udovicic force-pushed the detect-phpdoc-formatting branch 2 times, most recently from 8ce4351 to 8c154ae Compare July 2, 2019 08:34
@udovicic
Copy link
Member Author

udovicic commented Jul 2, 2019

Hi @lenaorobei ,

Code has been refactored to comply with the examples provided, unit tests have been adjusted accordingly and the name space has been changed to Commenting.

Copy link
Member

@larsroettig larsroettig left a comment

Choose a reason for hiding this comment

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

Hi thanks for your contributions looks nice currently we can not add new rules to the repo because of versioning rules it can take some time before it will be merged.

This is an adding new rule, so it will be the part of the next major release.
I need to suspend the processing of current PR since the versioning strategy is not clear yet magento/architecture#136

larsroettig
larsroettig previously approved these changes Jul 17, 2019
- added strict comparison
- changed sequence in the ruleset.xml
@lenaorobei
Copy link
Contributor

lenaorobei commented Jul 17, 2019

@udovicic thank you for this awesome contribution! 🚀
I added some minor fixes and going to merge the PR.

@lenaorobei lenaorobei merged commit 2a3889c into magento:develop Jul 17, 2019
@udovicic udovicic deleted the detect-phpdoc-formatting branch August 3, 2019 13:21
magento-devops-reposync-svc pushed a commit that referenced this pull request Nov 9, 2021
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.

4 participants