moodlehq / moodle-local_codechecker

A Moodle local plugin providing a simple web UI to run the MoodleCS coding style checks with PHP_CodeSniffer.
63 stars 72 forks source link

Mdef checking inconsistent #188

Closed marcusboon closed 2 years ago

marcusboon commented 2 years ago

I've got a file of constants (they're all just define(blah, blah)) with an mdef check at the top.

Code checker complains with:

--------------------------------------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
--------------------------------------------------------------------------------------------------
 25 | WARNING | Unexpected MOODLE_INTERNAL check. No side effects or multiple artifacts detected.
--------------------------------------------------------------------------------------------------

But if I remove the mdef check, it complains with:

------------------------------------------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
------------------------------------------------------------------------------------------------------
 1 | ERROR | Expected MOODLE_INTERNAL check or config.php inclusion. Change in global state detected.
------------------------------------------------------------------------------------------------------

constants.txt

stronk7 commented 2 years ago

Aha,

I think we implemented the logic for files having ONE or more "artifacts" (classes, interfaces, traits...). Surely didn't thought too much about those having zero artifacts and no side effects.

Thanks for the report, I'll try to reproduce (and fix it) soon.

CIao :-)

stronk7 commented 2 years ago

Hola @marcusboon,

I've created #191 and think that it will fix the problem reported here.

Basically, files that only contain non-relevant code (that by definition is code without side-effects too), won't need the MOODLE_INTERNAL check anymore and, if present, will report that it's not needed.

An example non-relevant code file is, for example this. That's the one that is being used in the tests, if you've anything else in yours, please let me know.

Ciao :-)

marcusboon commented 2 years ago

Hey @stronk7 ,

Thanks for looking into this and the test looks about right :)

Marcus