skohub-io / skohub-vocabs

A lightweight tool to publish SKOS Vocabularies
https://skohub.io/
Apache License 2.0
36 stars 25 forks source link

Add a contributing guidelines with a CONTRIBUTING.md #242

Closed sroertgen closed 1 year ago

sroertgen commented 1 year ago

This is a good starting point: https://github.com/metafacture/metafacture-core/blob/master/CONTRIBUTING.md

The Contributing guidelines should give a good overview and orientation regarding opening issues, reporting bugs, submission of enhancement suggestions as well as the development and review process.

There are some little differences between the metafacture guidelines:

Other than the metafacture guideline I propose a workflow where the person, who opens the pull request is also the assignee of the pull request. The idea is that this person is responsible for the discussion and progress of the pull request. E.g. if a reviewer forgets to the review or some other kind of issue arises during the PR the assignee is there to handle it and watch the progress. It also has the benefit that the PR remains visible for that person in related projects and boards.

Also I propose a workflow, where the issue remains assgined to the person originally working on it. In metafacture guidelines the issue is unassigned after functional review, because the idea is that everything is done there and only the PR is left since the issue will be closed automatically after the PR is merged. However I think there should be some kind of responsible person (the original assignee) who keeps assigned till everything is done and closed. In an ideal world everything should also work with an unassinged issue, but if, for whatever human reason, the issue is not linked or something else happens, the issue might maybe be forgotten since it does not show up in boards, projects or GitHub notifications.

Other than these small changes the guidelines are mostly the same.

In the section "Contributing Code" I added a small notice saying we use a test-driven approach and tests should be added to every feature / bug fix. I don't want to stress that point too much, because there is a lot of discussion about this topic, but it more or less boils down to the fact that a new feature should be added with tests that test that feature. Regarding bug fixes tests should be adapted to catch that bug. In general testing of behavior is favored over testing implementation details.

I'm happy and open for discussions, especially regarding the differences to the metafacture guidelines.

sroertgen commented 1 year ago

@Phu2 @acka47 I think you both should do the review of the guidelines:

https://github.com/skohub-io/skohub-vocabs/blob/242-contributingGuidelines/CONTRIBUTING.md

For SkoHub this especially means focussing more on the distinction between functional and code review. Currenty we do not assign functional reviewers and do both in the PR. We should also pay more attention to the definition of ready. I added a note about features that requires a small user story (if possible and sensible) so we get a more detailed picture of how that feature is going to be used. As well should a small check list be provided that makes it easy to know, when the requirements are met.

Phu2 commented 1 year ago

+1 The proposed changes sound reasonable to me.

acka47 commented 1 year ago

Other than the metafacture guideline I propose a workflow where the person, who opens the pull request is also the assignee of the pull request. (...) Also I propose a workflow, where the issue remains assgined to the person originally working on it.

I don't see this being clearly expressed in the current draft. For example this reads as if un- and re-assignments are expected:

Issues in working are only reassigned by the person who is currently assigned. If the assignee thinks the issue is ready for review they add instructions and links for testing the changed behavior in the issue, move it to the Review column, assigns the previously announced functional reviewer (see Definition of Ready), and open an pull request for the feature branch. The assignee of the pull request is the original assignee of the issue.

Here are some more thoughts I had while reading it:

acka47 commented 1 year ago

Also, I have a question re. the "Releasing SkoHub" paragraph: Is this something we already do or is it just a plan to do it like this?

sroertgen commented 1 year ago

@acka47 Thanks for the feedback.

Re: Assignment: I reworked the mentioned sections and hope it is now a bit clearer.

Re: Submitting enhancements I think you are right. I removed the two points.

Re: Deployment This should now be fixed with #217 A merge in main branch triggeres the built of a new docker image, which is then automatically pulled by skohub-webhook. Therefore vocabs are always built with the most current version now.

Re: Releasing SkoHub Vocabs I think we did not yet decide if we need releases. I removed that part for now.

btw: I moved discussion here, because in the first draft I copied the approach from metafacture that discussion of functional review should happen in the issue. And this is kind of a functional review. But after rework of that section I propose the approach that after an issue is ready and someone has a change ready and makes a PR the functional review and code review move to the PR. Therefore I will now open a PR and reassign you there.