search

FMCorz / mdk

Moodle Development Kit. A collection of tools meant to make developers' lives easier.
GNU General Public License v3.0
87 stars 47 forks source link

Replace references of `master` with `main` #221

Closed junpataleta closed 1 year ago

junpataleta commented 1 year ago

Additional notes:

See https://tracker.moodle.org/browse/MDLSITE-7418 for more information.

HuongNV13 commented 1 year ago

Thanks for working on this, Jun. The patch looks correct to me. I have tested the patch, including #222 and it worked for me. (Only for integration.git at the moment)

FMCorz commented 1 year ago

Thanks for the patch Jun. I think it's pretty good, although I feel that there may be some getting used for developers that aren't necessarily aware of the internal process at HQ. For instance, if someone upgrades MDK without the knowledge that mdk doctor must be run, they will be left with an MDK that is fairly broken, expecting the main branch in several instances and lead to variety of bugs.

Generally speaking, I try to keep upgrades as smooth as possible else users fear upgrading altogether. It would be fair for users to be fairly bothered if upgrading MDK rendered their development station somewhat unpredictable. For instance, the tools.stableBranch should ideally not return main until it has been confirmed that local instances have been updated to have a main branch. Now, this may be easier said than done, but that's my feeling.

This goes hand in hand with custom config values that are rendered ineffective because they have been renamed. We can't reasonably expect a user to know that these values don't apply anymore, and we don't have a good mechanism to advise that mdk doctor should be run (if that fixed the issue).

Lastly, I do not think anything should have a destructive behaviour. Keeping a master branch unused is probably fair enough, especially as the master branch is not disappearing from Moodle completely yet.

Let me know if I can be of further assistance with this change.

junpataleta commented 1 year ago

Thanks for the review, Fred!

I have updated the patch which is now taking a different approach and one that won't require users to call mdk doctor. In doing so, I have closed the related pull request #222.

Please see some points about the patch:

config keys

  1. Good one! I reverted the removal of wording.prefixMaster and wording.master. When calling mdk create, if either of these have been customised while wording.prefixMain and wording.main are at default values, the user will be notified about the new config settings replacing the xxmaster wordings and the value(s) of these xxmaster wordings will be copied to the xxmain wordings.
  2. I did not revert the removal of the masterBranch config setting this time though. Given that the comment about it in config-dist.json states that it should not be edited by users. If masterBranch is present in config.json though, while mainBranch is not, the user will be notified that masterBranch is being replaced by mainBranch and that it is recommended to remove it from config.json. The value of masterBranch will be copied to mainBranch and if --fix is provided, both mainBranch and masterBranch will be fixed, acordingly.

tools.stablebranch

I decided to keep it as it is, returning main when the version parameter is either master or main for the following reasons:

I created the fixup commit for easier viewing of what has been changed.

I hope that with these changes, developers can continue working as is after upgrading while getting informed of the new config settings.

Let me know if I missed anything or if there are things we can improve or other areas that need fixing.

Thanks!

junpataleta commented 1 year ago

Hi, @FMCorz! I have added an additional commit: https://github.com/FMCorz/mdk/pull/221/commits/ba75f2b28e32ebd79668979d8668c0b3375e9388.

I changed my mind about tools.stableBranch() and implemented the fallback mechanism there. But in order to do so, I had to pass a git object from the code calling it to enable the checking of the local remote for the presence of the main branch.

With this, the changes to moodle.checkout_stable() and moodle.update() in the fixup commit were reverted.

Here's a diff of the changes since your last review to make it easier for you to view the changes.

Once everything's okay, I'll squash the commits into one (or at least organise them better).

Cheers!

junpataleta commented 1 year ago

I've come up with a simpler version of the patch that provides support for the main branch and sets it as default but provides a fallback to the master branch. There are also no config changes in the patch so devs won't have to do anything after upgrading.

We can go with the rest of the renaming on a separate issue.

Feel free to check it out! https://github.com/FMCorz/mdk/compare/master...junpataleta:mdk:tomain_min

I'm happy to amend the current patch for this pull request with this minimal change should we decide that this is better.

Cheers!

FMCorz commented 1 year ago

Thanks Jun! The minimal patch looks great. I'd only recommend that we still allow --version master in the various commands that take this argument. Kudos for offering to re-use a previously created MDL-XXX-master in mdk fix.

junpataleta commented 1 year ago

Thanks, Fred!

I added kept master and added main in tools.version_options to keep supporting this version.

I also added another commit to support rebasing on master instances and branches.

FMCorz commented 1 year ago

Thanks a lot Jun!