grabs / moodle-tool_userdebug

3 stars 5 forks source link

Move config items into the normal location for plugins #10

Closed brendanheywood closed 6 months ago

brendanheywood commented 8 months ago

Some config items are stored eg:

$CFG->tool_userdebug_debugdisplay

These are in the 'core' config space and should be inside the plugin space. ie if you wanted to force these settings in config.php then you would normally do it like this:

$CFG->forced_plugin_settings['tool_userdebug']['debugdisplay'] = '1';
grabs commented 6 months ago

Hi Brendan,

thank you very much for your really helpful suggestions. This answer of me is related to the other issues (#6 and #8) too. I updated the plugin and did:

I hope you like it.

brendanheywood commented 6 months ago

hi @grabs

I'm going through all these changes now, but this one I cannot see.The last code still references this config in the global config space eg:

https://github.com/search?q=repo%3Agrabs%2Fmoodle-tool_userdebug%20tool_userdebug_debugdisplay&type=code

eg a function like this:

set_config('tool_userdebug_debugdisplay', $debugdisplay);

should instead be

set_config('debugdisplay', $debugdisplay, 'tool_userdebug');
grabs commented 6 months ago

Hi, there is a new branch MOODLE_404_STABLE which supports Moodle 4.1 to 4.4. You can find all things there.

brendanheywood commented 6 months ago

ah perfect sorry :)

A few meta bits of feedback:

1) is you add the issue # into the pull request it cross links between the issues and the code changes making it much easier to see what was actually change, see https://docs.github.com/en/issues/tracking-your-work-with-issues/linking-a-pull-request-to-an-issue

2) can you please make the new 404 branch the default in github?

grabs commented 6 months ago

Hi, the 404 is now the default. Unfortunately I did not understand your first point :(. I don't know what I could have linked to what.

brendanheywood commented 6 months ago

I mean when looking at this issue, there is no reference anywhere to the code which implements the feature and closes the issue. You can reference github issues in the git commits and you can also reference the issue in a github pull request and both of them will cross link which means if you are looking at a git blame then github can link directly from each line of code back to the issue that is was addressing, and you can link from the issue to the commit / pr.

Take this example issue:

https://github.com/catalyst/moodle-tool_excimer/issues/314

In the issue you can see that it was closed due to this pull request, because the PR description has #314 in it

https://github.com/catalyst/moodle-tool_excimer/pull/342

and in the code you can see the commit which fixed it because the git comment has #314 in it

https://github.com/catalyst/moodle-tool_excimer/pull/342/commits/32d6e25326c88a26b1eff5dfbc4420d813d4075c

Then if you do a git blame on a file you can see down the left all the different issues which relates to each change:

https://github.com/catalyst/moodle-tool_excimer/blame/MOODLE_35_STABLE/classes/web_processor.php

It's a pretty neat feature of github / gitlab and makes everything much nicer to jump around

grabs commented 6 months ago

I still don't really understand, because there is no pull request I could refer to. But you are right about the missing info in general. So I could just simply put the last branch or commit id in the issue answer, to guide the reader to the fixed version. Like this: Fixed in https://github.com/grabs/moodle-tool_userdebug/tree/MOODLE_404_STABLE

brendanheywood commented 6 months ago

The most useful and important thing is adding the issue number into the git commit comment. Using pull requests and cross referencing them is optional, but I personally find that its a much nicer workflow even when working alone with the CI / github actions setup. When working in a team it becomes even more useful