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

Downgrade to warning the new MoodleInternalNotNeeded error #174

Merged
merged 1 commit into from
Jan 17, 2022

Conversation

stronk7
Copy link
Member

@stronk7 stronk7 commented Jan 16, 2022

When the new detection was planned at #152, it was agreed
to make it a warning, not an error.

Instead, I missed that completely and implemented it as error.

So, this simple PR just downgrades the check to warning, as
originally planned.

With this patch applied, this will be the behavior:

  • Only files having side effects (changing global state) do require (error) the MOODLE_INTERNAL check to be added.
  • Files without side effects but having multiple artifacts (classes, interfaces and traits) "recommend" (warn) the MOODLE_INTERNAL check to be added.
  • Any other file without side effects "recommend" (warn) the MOODLE_INTERNAL check to be removed.

That behavior matches the agreement @ https://tracker.moodle.org/browse/MDLSITE-5967 plus add the new/remaining warning about unexpected MOODLE_INTERNAL checks.

When the new detection was planned at moodlehq#152, it was agreed
to make it a warning, not an error.

Instead, I missed that completely and implemented it as error.

So, this simple PR just downgrades the check to warning, as
originally planned.
Copy link
Contributor

@ilyatregubov ilyatregubov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed. Makes sense to me

@ilyatregubov ilyatregubov merged commit 801b053 into moodlehq:master Jan 17, 2022
@stronk7
Copy link
Member Author

stronk7 commented Jan 17, 2022

Thanks @ilyatregubov !

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