Hello! I had a few suggestions for improvements to the Contributor Checklist to make it more friendly to new contributors and hopefully reduce the amount of failed checks:
- In the first bullet point about linting and formatting, add a mention about
make lint / make fmt / make fmt-ui, or at least mention that various linting and formatting commands can be found in the Commands section of the Development page. The way that bullet point is currently worded, gofmt is the only formatter that is specifically called-out, so I didn’t realize that a specific formatter needed to be run for non-Go code. I had forgotten about the formatting commands on the Development page, so I manually ran a formatter for CSS, which apparently had different rules than make fmt-ui. A reminder about these provided linting/formatting commands in the checklist would have helped me.
- Add
make validate as a step in the checklist. Maybe as the last step on the list?
I think these two updates would quickly teach new contributors how to ensure they do not submit a PR that wastes the moderators’ time. If these updates are desired, I would be happy to write a new draft of the checklist for review.
2 Likes
I agree. I ran into these exact issues the first time I submitted a PR.
1 Like
Alright, I wrote a draft with my proposed updates. I also reordered the steps so that searching PRs & issues comes before any other work, and so that comments are added before formatting.
(Btw, I replaced the mention of gofmt with make fmt because I believe that’s just another way to run gofmt but without any ambiguity about which parameters should be used. I think.)
Contributor Checklist
Please make sure that you’ve considered the following before you submit your Pull Requests as ready for merging:
- I have read through formerly submitted Pull Requests and Issues to make sure that this contribution is required and isn’t a duplicate. Also, so that I can manage to close any Issues needing closed relating to this feature submission.
- I have adequately commented my code with the expectation in mind that anyone else should be able to look at this code I’ve submitted and know exactly what’s happening and what the expectations are.
- I have run code linters and formatters to make sure that my code is well-written and readable.
- Commands for Backend:
make lint and make fmt
- Commands for Frontend/UI:
make fmt-ui
- I have run
make validate to make sure that my code will pass all checks after my Pull Request is submitted.
Apologies for missing this. Were you still interested in doing a PR for this?
The draft above looks ok. My only comment is that it might be worth going into further detail on the last two points.
- for formatting backend:
make fmt
- for formatting frontend:
make fmt-ui
- to validate backend:
make validate-backend (this does the same as make lint it, the latter running the unit and integration tests)
- to validate frontend:
make validate-ui
- to validate both:
make validate
PRs that only change either backend or frontend need only run the applicable validatetarget.
I think it’s also worth adding a default pull request template. We have some old templates in there already, but they only apply when using specific query parameters in the new pull request URL.
Absolutely! Thanks for the feedback, here’s a new draft.
Some notes:
- I’m not super confident about bullet 3, and I’m not sure if it makes more sense to switch the order of 3 and 4.
- I couldn’t figure out a way to link to the PR templates that worked for forks (and I think that people with forks would be the primary audience for this checklist). I was going to include links to compare with the templates in the query params (like Compare · stashapp/stash · GitHub), but as soon as you switch the compare base to a fork, the query param gets dropped for some reason and the template won’t apply… Feels like a GitHub bug but I’m not sure. Anyway, I just linked to the plain-text view of the template files themselves, so people can just copy and paste them? It doesn’t feel great, but I think it’s better than asking contributors to find the right compare page and then manually add the query param.
Contributor Checklist
Please make sure that you’ve considered the following before you submit your Pull Requests as ready for merging:
- I have read through previously submitted Pull Requests and Issues to make sure that this contribution is required and isn’t a duplicate. Also, so that I can link or close any Issues relating to this feature submission.
- I have adequately commented my code with the expectation in mind that anyone else should be able to look at this code I’ve submitted and know exactly what’s happening and what the expectations are.
- If I made any changes to the back-end, I have run the
make lint command to check my code for any automatically-detectable issues.
- I have run the appropriate formatting commands (based on if I made changes to the front-end, back-end, or both) to make sure that my code is well-styled and readable:
- Format back-end:
make fmt
- Format front-end / UI:
make fmt-ui
- I have run an appropriate validation command (based on if I made changes to the front-end, back-end, or both) to make sure that my code will pass all checks after my Pull Request is submitted:
- Validate back-end:
make validate-backend
- Validate front-end / UI:
make validate-ui
- Validate both back-end and front-end:
make validate
- My Pull Request’s description follows one of these templates (copy and paste the file’s text into the description):