feat(templates): add PR and ISSUE templates #7
Conversation
Kirill-Vorobyev
requested review from
lpsinger,
Courey,
dakota002,
jak574 and
swyatt7
| ### IMPORTANT: Please do not create a Pull Request without creating an issue first. | ||
|
|
There was a problem hiding this comment.
| ### IMPORTANT: Please do not create a Pull Request without creating an issue first. |
This strikes me as an unnecessary speed bump when one identifies a bug and immediately crafts a fix.
There was a problem hiding this comment.
Although it may seem that way and I used to hold the same opinion about this, we must really ask why the process is currently so broken that someone would ever be in a situation where they had to exercise the power to immediately task-switch, diagnose, prioritize, plan, and complete a bug fix without involving the rest of the team in the issue triage, prioritization, design, refinement, work selection process.
If you're already comfortable doing all of these things on your own, create the issue for context and fill it out, it takes 5 minutes or less. It will greatly help your reviewers with understanding the what/why/how about the bug and clearly separate requirements from the work completed to meet those requirements.
We are all human and our interpretation of requirements can vary greatly, so keeping these artifacts separate strives to reduce ambiguity.
It also allows someone else to catch that the bug has been reported and that someone is working on it so there is no duplication of effort.
This gets into a deeper conversation about task-switching and work prioritization which we will have in due time.
There was a problem hiding this comment.
It will also create issues that can be tracked as bodies of work on the Projects board. That creates transparency and history.
There was a problem hiding this comment.
Overall, @Kirill-Vorobyev and @Courey, I agree with you. However, I think that we can skip that step for small or obvious bug fixes and typographical corrections.
There was a problem hiding this comment.
This is something I am unwilling to compromise on and is foundational to enacting a solid, transparent, and trustworthy SWE process. Every PR MUST have an associated issue. Every Issue MUST be prioritized and refined by the team before work can begin. There are many other problems with our process but we don't have the time to discuss them all here.
There was a problem hiding this comment.
I appreciate and respect that you have a strongly held conviction on this. I do as well, but in the opposite direction. I am prepared for you to convince me of your point of view, but it is not up to you alone to declare what we must do.
There is more than one valid software engineering tradition. I have certainly encountered many projects that have such a rule, and many others that don't. I think that the main predictor is whether the project is mainly corporate or academic. For example, numpy/scipy/matplotlib/astropy don't do this.
There was a problem hiding this comment.
Again, it's about traceability and accountability. If a developer creates "an obvious bug fix" and then gets hit by a bus with no documentation about what the fix was supposed to resolve in excruciating detail, then auditing the work that was done becomes much more difficult than it needs to be. What if that "obvious fix" also causes follow up issues or bugs in other places? What if it's only obvious to one person? No one can read minds, we must explicitly state all assumptions and requirements before work is started. Pull requests are also not the place to suggest how work should have been done to force a re-work. That is supposed to happen before work starts during planning and refinement. I don't have time to teach you the entire process in this comment, this is only a stepping stone.
I am not deciding this by myself, this is the result of collaborating with @Courey and @dakota002. It is the direct result of the discussions which you yourself have sought out for us to accomplish and simultaneously chosen to exclude yourself from in your slack post.
Please see the attached issue for more details, read the acceptance criteria which describes what qualifications this work should be judged by, and please refrain from derailing this PR any further.
ISSUE_TEMPLATE.md
Outdated
| - [ ] This is a bug report. | ||
| - [ ] This is a feature request. |
There was a problem hiding this comment.
Can we please use labels like
bug
There was a problem hiding this comment.
Good call! I also thought that coupling the task list feature of gh markdown was not the right tool for the job here.
| ### Description | ||
| A one to two sentence summary of the PR for easy digestion, must be different and more descriptive than title. Combination of title and description should summarize the issue + solution. |
There was a problem hiding this comment.
Can we merge Description and Changes?
Will this get filled in automatically from the long commit message?
There was a problem hiding this comment.
A description is distinctly different from the list of changes.
A list of changes dives into implementation details while a description keeps the higher level goals of the PR at the front of mind and provides a good summary without getting bogged down in details.
There was a problem hiding this comment.
Isn't the list of changes just the list of commits?
There was a problem hiding this comment.
sure, and if we're going to couple those concepts in our workflow and then enforce a conventional commits structure to our commits, we don't even need this section.
but from my past experience and current observations of commit messages in gcn so far, commits (names and descriptions) may not be truly reflective of their changes or may oversimplify many changes into a description, so this section is necessary to give the developer time to pause and organize their work and thoughts in a systematic way that reflects with brevity and clarity what was done as part of the PR in a detail oriented fashion.
a change list also allows us to confirm that the changes here are truly the only changes in the PR, more words and cross-references serve to improve overall code quality and simplify the review process for reviewers.
Try it, it might feel like a pointless time sink at first, but I assure you it's for improving everyones ergonomics in the long term.
There was a problem hiding this comment.
We have not inculcated a tradition of detailed commit messages. I think we should do that --- instead of listing out changes in the PRs.
There was a problem hiding this comment.
Clearly this is coupled with establishing a commit message style guide.
There was a problem hiding this comment.
Not necessarily. That is simply your interpretation. This is intentionally not coupled to commit messages to give us greater flexibility and provide momentum in the short term. We cannot tackle all of the things all at once. This PR with templates gets us one step closer in the right direction to establishing a typical SWE process.
|
Addressed all change requests that are in scope for this PR. |
There was a problem hiding this comment.
Some GitHub projects present you with a menu of several issue or PR templates. Can we do that instead of concatenating all of the options?
See https://github.com/remix-run/remix/issues/new as an example.
|
Yes we can, but that's outside the scope of this initial PR. |
Why? |
|
I'm confused what you're asking? Why is this out of scope? Because the acceptance criteria says it is. |
There was a problem hiding this comment.
@Kirill-Vorobyev, please watch your tone and take care that you are expressing yourself in a respectful manner. You have made offensive comments like this several times:
I don't have time to teach you the entire process in this comment, this is only a stepping stone.
I value your commitment to improving software process --- indeed, it's part of why we hired you. However, as I said, there are some important differences in culture, process, and etiquette between corporate software engineering projects and academic, scientific software projects. Because of how GCN and ACROSS are funded and because of who the external contributors are, we should take some elements of both traditions. Moreover, let us not disparage one or the other.
It is the direct result of the discussions which you yourself have sought out for us to accomplish and simultaneously chosen to exclude yourself from in your slack post.
I have been on leave for the past two business days and I am returning to work tomorrow; you can see that in my Outlook calendar. If I have not been an active participant in discussions on Slack, that is why.
Please see the attached issue for more details, read the acceptance criteria which describes what qualifications this work should be judged by, and please refrain from derailing this PR any further.
The value of writing detailed issues before starting PRs is that it allows us to gather input from stakeholders beforehand; having sufficiently specific requirements and acceptance criteria beforehand prevents us from doing unnecessary rework. I posted some feedback on #6 before you submitted this PR, but I don't see that that issue or this PR were updated to reflect that input. Namely...
From #6 (comment):
Issue template should not take more than 5 minutes to fill out. Pull request template should not take more than 10 minutes to fill out.
Does that include the time to craft a commit message or to write up a reproducer for a bug?As a matter of habit I tend to write pretty detailed PR messages that fully explain the changes and rationale. Ideally I would like a PR template that requires no more than a minute of additional data entry beyond that.
And from #6 (comment):
- This is a bug report.
- This is a feature request.
So, we already have the bug
Something isn't working and enhancementNew feature or request labels. Could we use those instead of checkboxes? It makes it easier to filter issues and PRs. Also, checklists look like to-do lists to me.
Let me remind you that this is your first PR here. Please show me that you can deal with criticism in a positive, constructive way. What I have learned from my experience working on open source software and publishing research papers with other people is that the best and quickest way to deal with criticism is usually to make the proposed changes. My feedback consists of the following:
- Add an issue template chooser rather than concatenating the various issue templates. See example from Remix.
- Automatically assign labels based on the selected template. See example from Remix.
- Remove the text, "IMPORTANT: Please do not create a Pull Request without creating an issue first." There are classes of PRs for which this is not appropriate: typographical errors and some external, organic, open-source contributions. I promise that we will incorporate this idea into our workflow in another way --- likely in the contributing guide.
|
Thanks for your feedback but it's out of scope for this ticket. Please create additional issues and add them to the backlog for prioritization and refinement by the team. Your suggestions are valid and reasonable. I have agreed with them throughout to show that your opinion is valued. We must adopt an iterative process and this is an exercise. If this doesn't work for you then self-prioritize the things you feel are important as you have been doing all along to get the things done that you wish to get done. PRs are not the place to stop the presses because you have a comment to make that places the entire context of work out of scope and makes it redundant forcing a rework unless it specifically presents a great risk to the project. The way templates are presented is not risky, so that means your comments are out of scope. Many teammates have already approved this PR so it will now be merged as stated initially.
|
|
@Kirill-Vorobyev, you knowingly merged this PR over my objections as the software development lead of GCN. I have reverted it. If you un-revert it, then I will revoke your committer access. If you continue arguing about this, then I will lock discussion on this issue. |
Description
New github Issues and PR templates are available and will be enabled automatically following merging this PR in all projects under the https://github.com/nasa-gcn organization
Reviewers
@Courey @dakota002 @swyatt7 @jak574 @lpsinger @Kirill-Vorobyev
Please review the templates for correctness and grammar.
Provide any suggestions to the templates so we can collaboratively iterate on their design.
Add any additional reviewers to this PR for context as you see fit.
Once we reach quorum on this PR with majority of stakeholders approving, we can merge and iterate on these templates as follow up issues.
Changes
Acceptance Criteria
Note: currently not possible to test because this initial issue/PR will add the templates.
Related Issue(s)
Fixes #6
Testing