search

NLESC-JCER / cpp2wasm

Guide to make C++ available as a web application
https://nlesc-jcer.github.io/cpp2wasm/
Apache License 2.0
22 stars 6 forks source link

notes on CONTRIBUTING #62

Closed jspaaks closed 4 years ago

jspaaks commented 4 years ago

I've collected some remarks about CONTRIBUTING.md. They are numbered so we can more easily discuss / decide / spawn new issues if necessary.

  1. CONTRIBUTING.md#L24-L32 might be covered by the bug template, maybe remove from CONTRIBUTING or just point to the bug template

  2. CONTRIBUTING.md#L48-L53 We could update the New feature issue template and remove the text here

  3. CONTRIBUTING.md#L63 I prefer the https link

  4. CONTRIBUTING.md#L104 this raises the threshold for making contributions. Maybe add something like "It's good practice to add tests when adding new functionality or changing existing functionality"

  5. CONTRIBUTING.md#L100-L111 We could move this whole part to a PR template file, that way people at least get to see it. I myself hardly ever follow the Check the CONTRIBUTING link that GitHub provides when making PRs.

  6. I believe this CONTRIBUTING.md#L117-L121 was just updated, we should update accordingly.

  7. We could wrap CONTRIBUTING.md#L120 in a script, e.g. dockerized-entangled. Perhaps we can even pass its arguments to inside the docker container. Or we could update the instructions to use make entangle instead.

  8. I would prefer to not have the git hook CONTRIBUTING.md#L136-L179, I find it surprising. As an alternative, maybe we can run the same functionality as a GitHub action; that way you get the check on code being in sync without surprising the user.

Let me know which of these suggestions you find useful, we can take it from there.

sverhoeven commented 4 years ago

I think note 1,2,3,4,5 are good suggestions and can be wrapped up in a single PR.

For 6, pandoc-tangle is still the preferred way to tangle. When entangled is made the preferred way in PR #53 then CONTRIBUTING.md should be updated accordingly.

For 7, make entangle is documented at L81, for literate programming we need someplace to have the Docker command. The best I can think of is to have in the tip1 chapter the docker command and give the make command as an alternative underneath.

For 8, The githook is optional so you can ignore it if you want to. In GH CI job we already check if everything is in sync. Performing commits in a GitHub action is a surprise to me. So perhaps making it clearer that the githook is optional would help you?

sverhoeven commented 4 years ago

Most points raised have been fixed in #97, can this issue be closed?

jspaaks commented 4 years ago

can this issue be closed

good with me