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

Check category tag #112

Merged
merged 2 commits into from
Feb 23, 2024
Merged

Conversation

andrewnicols
Copy link
Contributor

@andrewnicols andrewnicols commented Feb 19, 2024

Okay, so this adds a category check, which consumes the apis.json in the repo.

It currently doesnt' do anything fancy like checking the allowspread, or allowlevel2.

This is based on top of #110.

I don't know how we should handle disabling of the old moodlecheck rule here because apis.json has only existed since 4.2. Is it possible to disable the moodlecheck rule dependant upon a Moodle version?

Copy link

codecov bot commented Feb 19, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 97.15%. Comparing base (9e7d6f1) to head (491c94f).

Additional details and impacted files
@@             Coverage Diff              @@
##               main     #112      +/-   ##
============================================
+ Coverage     97.07%   97.15%   +0.08%     
- Complexity      633      655      +22     
============================================
  Files            28       29       +1     
  Lines          1809     1864      +55     
============================================
+ Hits           1756     1811      +55     
  Misses           53       53              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@andrewnicols
Copy link
Contributor Author

After discussion with Eloy I've found an approach which satisfies the 4.1 and earlier group. We're using a cached copy of the apis.json from Moodle 4.2 where the file was introduced.

The previous behaviour in moodle-local_moodlecheck was to query the devdocs for the current version so this is more accureate.

apis.json Outdated Show resolved Hide resolved
Copy link
Member

@stronk7 stronk7 left a comment

Choose a reason for hiding this comment

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

I think this is 99% ready, just have added a couple of comments. Also you've already created #115 so, once the comments are resolved... feel free to merge this.

Ciao :-)

moodle/Tests/Sniffs/Commenting/CategorySniffTest.php Outdated Show resolved Hide resolved
moodle/Util/MoodleUtil.php Outdated Show resolved Hide resolved
@andrewnicols andrewnicols merged commit e2fdd2f into moodlehq:main Feb 23, 2024
8 checks passed
@stronk7
Copy link
Member

stronk7 commented Feb 23, 2024

Yippee!

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