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

Make the sniff to also detect unexpected MOODLE_INTERNAL checks #169

Merged
merged 1 commit into from
Jan 5, 2022

Conversation

stronk7
Copy link
Member

@stronk7 stronk7 commented Jan 4, 2022

Until now, the Sniff was only finding missing MOODLE_INTERNAL checks
(when the file has side effects OR multiple artifacts).

Now, when the check exists and the file does not have side effects
or multiple artifacts, a new MoodleInternalNotNeeded is thrown.

Also, when the old MOODLE_INTERNAL check is detected, a new
MoodleInternalOld warning is thrown.

Covered with tests.

Finally, all the tests related with this Sniff have been moved
to a different test case and all the fixtures moved to new
subdirectory. Part of plans to split the tests completely, with
each sniff having its own testcase.

Fixes #152

@stronk7 stronk7 force-pushed the unexpected_moodle_internal branch from 674bc6e to 4768f96 Compare January 4, 2022 12:05
@stronk7
Copy link
Member Author

stronk7 commented Jan 4, 2022

Here there are the results of the Sniff against complete (master) codebase:

https://gist.github.com/stronk7/162e4b652490e02d66164606edb00864

9604 errors and 500 warnings.

Ciao :-)

@lucaboesch
Copy link

You have included the vendor directory, Eloy.

Until now, the Sniff was only finding missing MOODLE_INTERNAL checks
(when the file has side effects OR multiple artifacts).

Now, when the check exists and the file does not have side effects
or multiple artifacts, a new MoodleInternalNotNeeded is thrown.

Also, when the old MOODLE_INTERNAL check is detected, a new
MoodleInternalOld warning is thrown.

Covered with tests.

Finally, all the tests related with this Sniff have been moved
to a different test case and all the fixtures moved to new
subdirectory. Part of plans to split the tests completely, with
each sniff having its own testcase.

Fixes moodlehq#152
@stronk7 stronk7 force-pushed the unexpected_moodle_internal branch from 4768f96 to 2b019ce Compare January 4, 2022 15:01
@stronk7
Copy link
Member Author

stronk7 commented Jan 4, 2022

You have included the vendor directory, Eloy.

Yeah, and all third-party libs and others. Not critical. Neither the numbers are important or are going to be fixes in bulk. Still there are a lot.

Thanks!

@andrewnicols andrewnicols merged commit 06fb2c5 into moodlehq:master Jan 5, 2022
@stronk7 stronk7 deleted the unexpected_moodle_internal branch January 5, 2022 15:38
@danmarsden
Copy link

did this one really need to be an "error" state?
https://github.com/moodlehq/moodle-local_codechecker/pull/169/files#diff-a1e9c2ff7e45d5b92bf1baa5e465b8256f040c133fe1fad55a2da5536e3aeafaR125

I'm seeing this error in some pull requests this week on 3rd party plugins, and it doesn't really feel that important.. maybe it could have been a warning and then upgraded to an error at a later date?

@scara
Copy link

scara commented Jan 17, 2022

Hi @danmarsden,
FYI #174.

HTH,
Matteo

@danmarsden
Copy link

cool thanks - should have checked existing issues! :-)

@stronk7
Copy link
Member Author

stronk7 commented Jan 17, 2022

Yeah, my fault, the new check was incepted to be a warning but I forgot that completely and went the radical/error way.

Yep, #174 will, hopefully soon (planning new releases for as soon as today), fix that (thanks @scara).

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.

Start informing (warning) about not needed MOODLE_INTERNAL uses
5 participants