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

Remove category checks #132

Merged
merged 1 commit into from
Mar 7, 2024
Merged

Conversation

andrewnicols
Copy link
Contributor

We need to decide what we're doing here about Moodle 4.1 and earlier.
At what point do we remove these checks? Do we really need to check category on 4.1 and earlier given it's in sec support only now.

4.2 includes our new apis.json file.

@stronk7
Copy link
Member

stronk7 commented Feb 23, 2024

We have 2-3 options here:

  1. Proceed with the removal now (once the moodle-cs counterpart has landed). Moodle <= 4.1 will stop checking it.
  2. Keep the category checks as they are until the very end of the moodle-cs milestone and only remove it at the end. Moodle >= 4.2 problems will be reported by both tools.
  3. Adjust the category checks to only work for Moodle <= 4.1 (requires to inspect version.php and so on, not sure if, maybe, we are already doing that elsewhere). And keep it till the end. No dupes with moodle-cs.

If the 3rd alternative is going to be hard (although it may be a good general path to follow with a lot of checks), then I'd say that the 2nd one is my personal best one. Better report twice than none.

Ciao :-)

@stronk7
Copy link
Member

stronk7 commented Feb 23, 2024

This (or whatever we agree to do) will be applied after moodlehq/moodle-cs#112 has landed.

@andrewnicols
Copy link
Contributor Author

After thinking things through, we are including the 4.2 version of the apis.json into the moodle-cs repo for versions which do not have the file. This can land and we don't need to do any version checks etc.

@andrewnicols andrewnicols marked this pull request as ready for review February 23, 2024 09:14
From Moodle 4.2 these are performed with moodle-cs.
@stronk7 stronk7 merged commit 49d5091 into moodlehq:main Mar 7, 2024
20 checks passed
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