Resolve CodeChecker compliance

emmarichardson / local_autogroup

A local plugin for Moodle 2.7 onwards which handles the dynamic creation, population and cleanup of groups on courses.

6 stars 19 forks source link

Resolve CodeChecker compliance #40

Closed Kemmotar83 closed 1 year ago

Kemmotar83 commented 1 year ago

Code refactoring for Code Checker compliance #39

ak4t0sh commented 1 year ago

Thanks for sharing. As I thought that's a "big" PR to review ^^" I'll try to review it next week but in the mean time could you sum me up your changes please ? Indeed your commit only specify "code checker compliance" but it seems you have done other changes. Ideally you should have done at least 2 commit. (1 for your changes and 1 for code checker compliance) :) But if you could sum me up your changes to help me understand your aims that would be fine to me ;)

TIA

Kemmotar83 commented 1 year ago

Hi @ak4t0sh ,

in this commit I've done only refactoring for code checker compliance, no other feature/fix changes. It seems a lot of stuff but it is only code reformat / rearrange, with some other changes in order to make Code Checker happy (ie. __invoke methods). Changes in CHANGES.md are only to fix missing realase data from the previous release (2.7), but you can discard those changes if you want.

To help your review, this isn't a totally manual refactoring. I've used PHPStorm code correction functionalities to reformat and rearrange code using Moodle standards. Then I've manually fixed comments and other minor fix.

I've also done some manual testing, but I haven't a Totara installation, so I cannot test Totara specific features. For the future, it could be helpful to have automatic tests for a straightforward CI.

Giorgio

ak4t0sh commented 1 year ago

Thanks for your clarifications. I understand my mistake...I see that you reoderred methods too that's after only a quick look on the diff I thought you added functions ! I'll take a more precise look later but at first sight It seems OK.

emmarichardson commented 1 year ago

@ak4t0sh Have you had a chance to review this?

emmarichardson commented 1 year ago

Preliminary testing on dev site shows no issues. Only errors showing that I have seen are related to Issue #20 which I have replicated.

ak4t0sh commented 1 year ago

Was working on it last night :) Did not have time to test it. Just reviewing the code Its OK for me. Just few notes:

Thanks for adding missing entry in CHANGES.md (@emmarichardson when you release new version try to think to update CHANGES.md it will help admin to now what changes are expected ;) )
I do not understand why you replaced __invoke() usage by a "standard" invoke method. Can you explain me the reason for this change please ? (Anyway I think this is more understandable for new contributors I would not set this a a blocker for merging)
Another change to comply with moodle standard would be to rename functions defined in locallib.php using frankenstyle prefix. @Kemmotar83 do you think you could add this changes to this PR please ?

Kemmotar83 commented 1 year ago

Hi @emmarichardson , I've only done refactoring so previous bugs are still there.

Hi @ak4t0sh , I've replaced __invoke declaration because I had warning messages from CodeChecker about not legal use of magic methods. For locallib.php do you want to add localautogroup prefix functions? This is not strictly related to CodeChecker compliance but it's ok, I'll do that immediately.

Giorgio

ak4t0sh commented 1 year ago

Thanks for your reply concerning __invoke.
concerning function naming I did not see that they are in a namespace...weird...could your remove it please ? https://docs.moodle.org/dev/Coding_style#Functions_and_Methods

otherwise it seems good to me for merge

Kemmotar83 commented 1 year ago

Hi @ak4t0sh

I misunderstood your message and I reverted the last commit. Tomorrow I'll fix and push the final version.

Thanks

Kemmotar83 commented 1 year ago

Hi, I made a little mess with git but now it should be ok. Luckily you can squash when merging.

I need to remember that after a day of work I have to read carefully instead of trying to do a fast fix.

ak4t0sh commented 1 year ago

Thx for your work. Look good to me. @emmarichardson if your tests are OK you can merge this PR (use squash commit to keep a clean history)

emmarichardson commented 1 year ago

Thank you for all the work on this. I have merged this commit into the code. I will wait to update the plugins database to see if we can get the actual functionality changes done. @Kemmotar83 if you could include the other pull request (it just a couple of lines of changes) in your next pull request, that would make things a lot simpler as that pull request is an older version and I was unable to get it to pull to test.

Kemmotar83 commented 1 year ago

Hi @emmarichardson, I had the fix ready but I haven't done any other pull request yet. I'll do it immediately.