Remove requirement of issue reference from pre-commit hook? about pluslib HOT 10 CLOSED

Hmm, I know it's sometimes a hassle, but I think it's worth keeping.

We can implement a server side solution via their webhooks. Although you can't reject a push, you could lock down the master branch, and only allow pushes to development branch, and have a CI server merge to master after a CI build passes.

I know it's a bit of effort to set up a new build system, but I think it's worth it.

We could also have the CI setup analyze the commit message with some logic, and auto-accept based on criteria, or flag an admin (you, me, Csaba) to manually create a correct commit message. Again, I know it's more maintenance for us, but in my opinion this is the cost of running a project.

Although I can't at this moment, this is something I would be willing to work on after my comprehensive exams are done.

from pluslib.

Addendum: since we've moved to github, we've gained the fact that there is a huge community of third-party integrations that make it relatively easy to add nice things like build status icons, etc... to our readme.md page.

from pluslib.

Additionally: I just realized... we can run the SetupForDevelopment.sh from the CMake configure step! This would at least enforce some rules. We could also improve the pre-commit hook by adding valid ticket commits (open tickets only, etc...)

from pluslib.

Third party integrations are all good, we should definitely make use of them - but let's get back to that later, it's not related to this topic.

Doing checks in pre-commit hook and "enforcing" them through CMake is a good idea, too.

However, it's not clear for me what we gain by forcing references to issues. On Assembla/SVN it was essential, as that was the place where discussions could happen. However, now we have pull requests where we can discuss issues, and commits are automatically associated with it (through the topic branch the pull is requested from).

For many small fixes that we integrate directly, without discussion, often have been integrated with "fake" ticket references anyway (see for example your "HoloLens" ticket, but I did similar things, too).

Somewhat related issue is that ticket numbering is restarted on github. I don't know if we can do something to force github to start numbering tickets from a higher starting number so that we know if the ticket is on github or it was on Assembla.

from pluslib.

I tend to agree with @lassoan. I have mainly worked on Slicer core in the past year or two, and I did not feel that always having to reference an issue was necessary.

Many times, especially COMP, STYLE, and PERF type commits had no reason to be associated to a ticket, while in SlicerRT for example, if I had such a change to commit, I had to create a ticket just to be able to make that one commit, or I created a generic ticket that I always referenced (like all tickets in the Continuous milestone).

Also, for projects where there is a lot of collaboration, the tickets that are related can come from multiple systems. For example for Slicer core, we have the Slicer Mantis, but we also have many projects that involve a lot of Slicer core work (QIICR, SlicerRT, etc.). And Plus is such a project, because many times SlicerIGT or PerkTutor bugs have to be fixed in Plus.
Besides this, often the commit in Slicer core is simply related to a Discourse thread.

So I don't think that enforcing this is a good idea, or even viable, see above about community contributions and merge commits.
If we do add a pre-commit hook for the GitHub repo, then I think it should be enough requirement that one URL must be in the commit message. I'm not sure if it's possible for external repositories, but it would be nice that if somebody pastes a GitHub issue URL, then the commit shows up in the thread of that issue.

from pluslib.

Thanks Csaba, good points.

I just remember that another reason for enforcing ticket references was to prevent users from negligently or accidentally write poor commit messages (in Assembla SVN, commit messages were not possible to change retrospectively). We don't have this problem on github.

from pluslib.

Makes sense. I'm on board with removing it. We could keep a few basic rules (like min 40 chars or something).

from pluslib.

A character limit is definitely a good idea. I'm not sure if a URL requirement is feasible (like when automatic merge commits there is none), but it would be useful I think.

from pluslib.

I like the topic branch/pull request flow. @lassoan feel free to remove # requirement.

from pluslib.

I've removed the requirement of referring to issue number from PlsuBuild, PlusLib, PlusApp repositories.

from pluslib.