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

Add sniff to check for first line descriptions #134

Merged
merged 1 commit into from
Mar 27, 2024

Conversation

andrewnicols
Copy link
Contributor

@andrewnicols andrewnicols commented Mar 23, 2024

Replaces:

  • phpdocsfistline
  • functiondescription

Replaces moodlehq/moodle-local_moodlecheck#142

Copy link

codecov bot commented Mar 23, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 97.87%. Comparing base (5af7ee0) to head (ad8be34).

Additional details and impacted files
@@             Coverage Diff              @@
##               main     #134      +/-   ##
============================================
+ Coverage     97.84%   97.87%   +0.03%     
- Complexity      835      844       +9     
============================================
  Files            36       37       +1     
  Lines          2463     2499      +36     
============================================
+ Hits           2410     2446      +36     
  Misses           53       53              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@stronk7
Copy link
Member

stronk7 commented Mar 25, 2024

Q: Does this cover cases like:

/**
 * This is the first line of the block.
 * But the second an following aren't separated enough and
 * make the block to look weird.
 */
<<files, classes, ..., but methods, that I think don't require it, go here >>

I mean, it sounds to me that the 1st line alone (with empty line after), has been always a requirement for them. But we haven't enforced it, I think. And this may be the perfect sniff for that.

andrewnicols added a commit to andrewnicols/moodle-local_moodlecheck that referenced this pull request Mar 25, 2024
@andrewnicols
Copy link
Contributor Author

Q: Does this cover cases like:

/**
 * This is the first line of the block.
 * But the second an following aren't separated enough and
 * make the block to look weird.
 */
<<files, classes, ..., but methods, that I think don't require it, go here >>

I mean, it sounds to me that the 1st line alone (with empty line after), has been always a requirement for them. But we haven't enforced it, I think. And this may be the perfect sniff for that.

It doesn't cover that case right now, but it's not hard to add. Still, I'd prefer to do it as a separate PR in this case because there are a couple of ways to do it.

@stronk7
Copy link
Member

stronk7 commented Mar 27, 2024

Still, I'd prefer to do it as a separate PR in this case because there are a couple of ways to do it.

Agreed, we can create a new issue with some thoughts about what we may need to check:

  • Blank line between summary (short desc) and description (and for which elements).
  • Blank like between descriptions and tags.
  • Maybe some ordering and spacing between tag "groups" (copyright, author, license => params => return => other tags)
  • ....
    (I'd look to what the phpdoc proposal says for anything not explicitly defined in our guidelines)

I'll proceed to review this soon... ciao :-)

@stronk7
Copy link
Member

stronk7 commented Mar 27, 2024

Have created #136 to have the discussion above registered.

@stronk7 stronk7 merged commit ab43d49 into moodlehq:main Mar 27, 2024
8 checks passed
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.

2 participants