Move Moodle phpcs rulesets to external repository

[andrewnicols]

Using git subtree to move the Moodle Coding Style rules to their own repository.

This does change the path, and lead to inclusion of additional files, but should be safe.

This change includes a commit to add to readme, and update the internal Codesniffer config to use the new location.

[andrewnicols]

Unit test failures are caused by https://tracker.moodle.org/browse/MDL-74044

I'll look into the Behat failures tomorrow.

[andrewnicols]

As discussed via chat, I've restructured the rules in the moodlehq/moodle-cs project to not put them into an extra unnecessaray phpcs folder.

I've put them here in a rules folder to make git subtree happy and more predictable - otherwise we end up with rules in moodle/moodle. Now they re in rules/moodle.

[stronk7]

Maybe we should skip from the "copy" (git subtree) all the moodle-phpcs tests? No way they are going to work within the plugin anymore (different namespaces, non-standard - for plugins - directories...).

So local_codechecker just will have a few behat and phpunit OWN (main /tests directory within the plugin) tests, verifying that the integration works and done.

Edited: And, maybe, if we are going to exclude some stuff in the copy... also remove git/composer/travis/gha/readme... maybe also license if we add it to local_codechecker...

[stronk7]

Have added one little commit removing now unused based testcase (it belongs to moodle-cs and is already there) and adjusting the 2 failing behats to point to different, more stable directory (file-count wise).

Ciao :-)

Edited: Have also added the "rules" directory (I'm not 100% sold about needing that rules/moodle, but that's another story) to the thirdpartylibs file. That makes codechecker and moodlecheck to automatically ignore it (good), but makes some behat tests to fail (surely we'll need to change them to avoid using the rules dir at all).

[stronk7]

Ok, have amended the extra commit commented above:

1. Added the moodle-cs directory (rules/moodle right now) to thirdpartylibs.xml.
2. That makes both codechecker and moodlecheck own tests to pass (the dir is ignored).
3. Amend the behat tests to use own files as fixtures, instead of using the moodle-cs standard ones.
4. Removed base testcase that now belongs to moodle-cs.

With these changes, we should get everything passing BUT phpunit tests (that still tries to run the moodle-cs ones and, obviously, they don't work anymore with Moodle.

So, IMO, this is really ready. As commented elsewhere the only 2 points worth agreeing are:

i. is rules/moodle a good target directory? Wouldn't be better to create a MoodleCS one (that is the name of the standard) and put everything straight there? ii. Maybe, if we are going to keep some stuff out from the copy from moodle-cs, instead of using git subtree, we can change to simpler copy and forget? We are already copying both the "phpcs" and the "PHPCompatibility" directories, so also copying the "MoodleCS" one shouldn't be a problem (excluding tests and other stuff we don't need within codechecker). iii. Future improvement, surely we can do the build phase automated (Say Makefile/CI or whatever). But it doesn't need to happen here. Let's do the first split release first.

Thoughts? Ciao :-)

[stronk7]

Hi @andrewnicols, I've added one more commit, labeled as WIP, that basically moves your git subtree (in the "rules" directory) to be a simple cp -pr plus manual removals (in the "MoodleCS" directory).

Have changed the readme_moodle.txt and other small bits accordingly.

How does that sound? Everything seems to be passing now and I think it's not a big problem to use the "copy" alternative (plus removals), as commented above. In the future we may automate everything easily too.

Ciao :-)

[andrewnicols]

As commented in chat, I think that sounds fine :)

[stronk7]

Only reflexion that comes to me is that, as part of this PR... we are removing the "moodle" directory, where the Moodle standard is sitting right now. Can imagine that breaking a number of integrations.

• CiBoT (local_ci) and moodle-plugin-ci both need to be checked.
• People pointing their IDEs to old directory will stop working too (I've shared in the Dev chats about the move and recommend everybody to go switching to MoodleCS).
• Other integrations...

So we need to try to minimize the impact. Maybe:

1. Keeping the old "moodle"directory around for some time (a couple of months?).
2. Making CiBoT and moodle-plugin-ci to support both the new and the old directories.
3. Making CiBoT to inform about that in all runs.
4. Making moodle-plugin-ci release, announcing the change too.
5. Integration exposed (N times).
6. Moodle Dev Chat (N times).

The important decision is 1), if we keep the old standard over some time, we'll have time to change integrations and warn N times.

Ciao :-)

Edited: To remove the wrong heading, sorry!

[stronk7]

I'm going to try creating a soft alias from, now gone, "moodle" to new "MoodleCS/moodle". Maybe that's enough for places (IDEs) using the former to continue working perfectly with the later.

Also will check how both CiBoT and moodle-plugin-ci support this change, though ideally we should be moving both them to use Moodle-CS new standard and remove their local_codechecker dependency.

Let's see...

[stronk7]

Both ViM and PHPStorm continued working perfectly with the "moodle" alias pointing to "MoodleCS/moodle". And, if using the bundled codechecker, no need to specify any installed standards paths, because they are taken from the phpcs/CodeSniffer.conf file.

[stronk7]

For the records, these are the changes I'm aiming to have deployed before getting this in:

1. ✅ Make local_ci (CiBoT) to use moodle-cs instead of local_codechecker. PR available #248.
2. ✅ Make moodle-plugin-ci to use moodle-cs instead of local_codechecker. PR available #171.
3. ... (haven't been able to imagine any other dependency on current local_codechecker, if you can, please, tell).

So, once all the dependencies above are removed, this will find its route without any impact to others (already switched to moodle-cs).

Ciao :-)

[stronk7]

With moodle-plugin-ci 3.3.0 already released, we have now killed the dependency #2 above, so nobody is using local_codechecker as source of the standard anymore.

Only user pointing to it from IDEs will.

So I'm going to:

1. Give to this a final review.
2. Add a "moodle/" alias, pointing to the new "MoodleCS/moodle" place, as commented above, so people pointing to the old location will have a chance of it to continue working.
3. test, test, test

Ciao :-)

[stronk7]

Hi @andrewnicols,

finally got some time to look to this... wanted to ask/discuss...

1. Basically I'm looking about to revert #186. Now that we have moodle-cs, surely we can point to it in the README, and people wanting IDE integration and so on, just use it. As far as local_codechecker is becoming just "one more consumer" of moodle-cs, I think it's the correct thing to do.

2. What I was thinking... is to keep moodle-cs as a "dev" dependency... so whenever we want to bump the version bundled here under MoodleCS/moodle... all we have to do is to get the latest via composer and make a directory replacement (from vendor/moodlehq/moodle-cs to MoodleCS), commit everything and done). <<== Abandoned this, it's pretty easy to perform the updates from local clones, maybe some day in an automated way too.

In fact, I'm doing that now to add to local_codechecker all the recent modifications (already released) that we have added to moodle-cs recently. I'll write an UPGRADING.md file to have the process defined.

What do you think about both the 1. revert, pointing to moodle-cs and 2. use dev dependency for easier upgrading.

Ciao :-)

[stronk7]

Ok,

I think this should be considered ready, both GHA tests and local testing (web + cli) are returning the expected results.

Summary of changes:

1. This is, right now, on top of current master.
2. Commits are self-explanatory, here it's a short summary:
   1. delete the old moodle directory containing the standard.
   2. import moodle-cs via git subtree (@ /rules)
   3. amend tests and thirdparty info to keep local_codechecker own tests passing.
   4. switch from git subtree (@ /rules) to simple copy from moodle-cs (@ /MoodleCS).
   5. add a /moodle alias pointing to new /MoodleCS/moodle (so any integration pointing to the old dir will continue working for 1 year, then the alias will be removed).
   6. Stop suggesting any composer or IDE integration with local_codechecker. Instead point to the moodle-cs repository (the new source of the coding style).
   7. Upgrade bundled PHP_CodeSniffer to 3.7.1
   8. Upgrade bundled moodle-cs to current 3.2.4+

And that's all. Next steps should be:

1. @stronk7 approves this.
2. @andrewnicols reads this message and takes a look to see if there is anything else to change.
3. This is merged (anybody)
4. @stronk7: A new v4.0.0 release is published, informing in all the channels and encouraging people to go switching their integrations to moodle-cs (and reporting issues there too).

Yay, end of the migration to moodle-cs done!

Future: Create some workflow/pipeline in order to, automatically, get any PHP_CodeSniffer, moodle-cs or PHPCompatibility release, applied to local_codechecker via PR. In the mean time, we'll be doing it manually following the instructions in the readme_moodle.txt file.

Ciao :-)

[stronk7]

Yay, thanks... heading to prepare the v4.0.0 release in 24h... yippee!