Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

notes on CONTRIBUTING #62

Closed
jspaaks opened this issue Jun 5, 2020 · 3 comments
Closed

notes on CONTRIBUTING #62

jspaaks opened this issue Jun 5, 2020 · 3 comments
Assignees
Labels
enhancement New feature or request

Comments

@jspaaks
Copy link

jspaaks commented Jun 5, 2020

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.

@jspaaks jspaaks added the enhancement New feature or request label Jun 5, 2020
@sverhoeven
Copy link
Member

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
Copy link
Member

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

@jspaaks
Copy link
Author

jspaaks commented Aug 17, 2020

can this issue be closed

good with me

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants