-
Notifications
You must be signed in to change notification settings - Fork 26
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
Update Moodle coding standards #128
base: MOODLE_401_STABLE
Are you sure you want to change the base?
Conversation
Menrath
commented
Jan 30, 2024
- update CI to latest distribution version
- Update Moodle-Plugin-CI to version 4
- apply phpcbf with moodle coding standard
- add coverage information to tests
- define class properties in board_table
- import core_external functions
- add Moodle 4.3 to CI with PHP 8.2
update CI to latest distribution version apply phpcbf with moodle coding standard add coverage information to tests define class properties in board_table import core_external functions add Moodle 4.3 to CI with PHP 8.2 Update Moodle-Plugin-CI to version 4 and make tasks more strict according to the recommendations fix coding style eslint warnings allow eslint warnings for CI rebuild javascript revert CI to v3 update CI to latest gha.dist https://github.com/moodlehq/moodle-plugin-ci/blob/main/gha.dist.yml rebuild js
Hi @Menrath , apologies for only getting to review this contribution now. All of your code changes appears robust and ready, however there is the matter of the conflicts to be updated. We are planning to do a new release for the 4.5 major release date, so if you have time to get this PR ready in the next week, we'd be happy to review again with a view to integrating it. Many thanks in advance |
@learningtechnologyservices Hello, one question: is MOODLE_401_STABLE still the latest branch? |
@learningtechnologyservices Synced with latest branch and ready for review again. Propably MOODLE_404_STABLE and PHP 8.3 should be added to the CI. |
But I think adjusting this is maybe out of scope of this PR? |
Hi André Apologies for this message being very direct, but we are incredibly buzy here at the moment. On the PR itself, we need to move urgently to get our release done this week (ideally tomorrow) for 4.5 early bird status. We are happy to integrate the ci.yml file changes as a separate PR immediately. We would need to spend a lot more time going through the rest of the changes, which are very welcome but also a lot to review, so we would aim to do a second release soon after to include these other changes. Also, the changes for adding class variables here would need to be removed from your PR as well. We appreciate your collaboration in this board update, and hope to see these requested two separate PRs for us to test, review, and integrate. |
Hi @Menrath , meant to ping you on previous comment. |
@Menrath , also on the branches, we are planning the 401 release with the latest we have here (plus your CI change up to 403 hopefully). We will also release a separate 404/405 release, which will be the 401 code plus the icon PR here! |