search

moodlehq / moodle-cs

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

Check package tags #110

Closed andrewnicols closed 6 months ago

andrewnicols commented 7 months ago

This is a first attempt at migrating one of the checks in moodlecheck as part of #30

This change starts the work required to migrate the @package check.

There's a lot of duplicated code here right now, and a lot of missing tests, but I've tried to abstract out useful features or use phpcsextra where possible.

codecov[bot] commented 7 months ago

Codecov Report

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

Project coverage is 97.06%. Comparing base (61a380a) to head (dbbffda).

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #110 +/- ## ============================================ + Coverage 96.73% 97.06% +0.32% - Complexity 586 632 +46 ============================================ Files 26 28 +2 Lines 1625 1807 +182 ============================================ + Hits 1572 1754 +182 Misses 53 53 ```

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

stronk7 commented 7 months ago

I won't merge this, coz my #111 will conflict then (I imagine, coz both are moving MoodleUtilTest)!

(joking, of course)

andrewnicols commented 6 months ago

Confirmation: Does this cover all the related tests and fixtures that we have in local_moodlecheck. Or, with other words, is this a complete replacement?

From what I can see in moodlecheck, there is very little testing of the package tests. See https://github.com/moodlehq/moodle-local_moodlecheck/blob/main/tests/moodlecheck_rules_test.php - there are 7 mentions of the packagevalid string, bug they're just checking that an error was raised in unrelated tests. There are no specific tests, nor any tests which check that it's in the right place.

As far as I'm aware, this is a complete replacement. There are two rules for packages:

The specified check notes:

Checks if all functions (outside class) and classes have package package tag may be inherited from file-level phpdocs

The new checks do the same thing.

The valid check notes:

Checks that wherever the package token is specified it is valid

It does this by checking the package against a list returned from local_moodlecheck_package_names. That list basically comes from \core_component, as does the new test.

Maybe, getting used to feed the CHANGELOG as part of the PR is good thing.

Bah - yes. I must get in this habit (done).

And, final reflexion... I think I commented elsewhere... let's give to this initiative a little bit of visibility (chat, forum, integration exposed, ...). Maybe there is something in the plan that we haven't detected, or somebody has a say about it...

Yes - you suggested this in PM. I'd say let's do:

stronk7 commented 6 months ago

Cool all!

andrewnicols commented 6 months ago

Announced here: https://moodle.org/mod/forum/discuss.php?d=455786