moodlehq / moodle-cs

Moodle Coding Style
https://github.com/moodlehq/moodle-cs
GNU General Public License v3.0
18 stars 16 forks source link

Add sniff to check for first line descriptions #134

Closed andrewnicols closed 8 months ago

andrewnicols commented 8 months ago

Replaces:

Replaces https://github.com/moodlehq/moodle-local_moodlecheck/pull/142

codecov[bot] commented 8 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

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

Additional details and impacted files ```diff @@ 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 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

stronk7 commented 8 months ago

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 commented 8 months ago

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 commented 8 months ago

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:

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

stronk7 commented 8 months ago

Have created #136 to have the discussion above registered.