search

daledavies / moodle-tool_registrationrules

GNU General Public License v3.0
2 stars 1 forks source link

CI/pipeline fixes #17

Closed phager-at closed 2 months ago

phager-at commented 2 months ago

Fixes #10 , #9

daledavies commented 2 months ago

Niiice

phager-at commented 2 months ago

MR is ready for review. It focuses on the CI and pipeline fixes. And I'm sorry, but it's gotten really enormous—I pity the reviewer by now. :grimacing:

phager-at commented 2 months ago

Maybe it's best reviewed commit-by-commit to stay manageable.

daledavies commented 2 months ago

Not a problem, thanks for this! As expected it conflicts with my recent changes in main but shouldn't be anything major.

daledavies commented 2 months ago

I'm doing it file by file at the minute, we'll see how that goes. I've been working on some changes ready for implementing scoring so would be nice to have this merged asap.

daledavies commented 2 months ago

Hah many of your TODOs I've started to address too, not created an issue for it as I got carried away but things like addressing the differences between rule_checker::get_messages() and get_validation_messages()

daledavies commented 2 months ago

Looks good, nice work!

Just a few minor nitpicks really, I'm a native English speaker but my grasp of the English language is far from perfect so feel free to ignore.

phager-at commented 2 months ago

Looks good, nice work!

Just a few minor nitpicks really, I'm a native English speaker but my grasp of the English language is far from perfect so feel free to ignore.

Looking forward to incorporating your feedback! My peak of comprehension of the English language lies about 20 years (± a few years) in the past, when I took the FCE during school. :-)

I'll try to go through your nitpicks and rebase some time today! And if you've got any more nitpicks for me, feel free to add them too. I'm not the best in expressing things in an understandable way.

daledavies commented 2 months ago

I appear to have pressed a button and requested a new review from myself!

phager-at commented 2 months ago

@daledavies I finally found time to implement your feedback and rebase the branch! So if all checks pass we're ready to follow up on your review!

Do you usually have the developer close the threads as soon as they're processed or do you let the reviewer close them? We usually let the reviewer close them, so he/she has the chance to re-review the incorporated feedback too.

daledavies commented 2 months ago

Awesome, we've got a well defined process around gitlab but rarely use github internally. I think having the reviewer close the conversations would be closest, and then give some form of approval for the PR to be merged.

As we use gitlab we're talking about merge requests but we'd normally have the developer proceed with the actual merge once the MR has been approved by the reviewer.

daledavies commented 2 months ago

All checks have passed!

daledavies commented 2 months ago

Having said the above about the reviewer merging, I went ahead and merged as I can now push my own changes :)

Thank you for all this :)