chore: remove plugin checklist from PR template #7105
Conversation
simon-id
changed the title
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #7105 +/- ##
=======================================
Coverage 84.76% 84.76%
=======================================
Files 521 521
Lines 22151 22151
=======================================
Hits 18776 18776
Misses 3375 3375 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Overall package sizeSelf size: 4.34 MB Dependency sizes| name | version | self size | total size | |------|---------|-----------|------------| | import-in-the-middle | 1.15.0 | 127.66 kB | 856.24 kB | | dc-polyfill | 0.1.10 | 26.73 kB | 26.73 kB |🤖 This report was automatically generated by heaviest-objects-in-the-universe |
This comment has been minimized.
This comment has been minimized.
| ### Additional Notes | ||
| <!-- Anything else we should know when reviewing? --> |
There was a problem hiding this comment.
Why remove the notes section?
There was a problem hiding this comment.
Because I find it superfluous. There are two possible cases:
- you don't have any additional notes, and thus you need to remove this section manually.
- you have additional notes, and do I REALLY need to remind you that you're allowed to add additional notes to your PR description ? like if you wanna write more stuff, just write more stuff.
wdyt ?
There was a problem hiding this comment.
I agree people will find a place to put it regardless, but I think it's nice to have a place that's dedicated so the its information isn't accidentally mixed in with to the two other sections. E.g. I usually use the section for information relevant to the reviewer only. Recent examples:
- feat(debugger): add special handling for very large collections/objects #6912
- fix(otel): support setting DD_METRICS_OTEL_ENABLED to False #7028
- vendor bundled non-datadog dependencies #6958
- [SVLS-7168] Create inferred Span and Span links for GCP PubSub #6415
- [SVLS-7168]Support GCP PubSub Batch Publish #6782
- switch to js implementation of orchestrion #6877
There was a problem hiding this comment.
Yes that makes sense, and as my previous message, that falls into the second case, if you have it, you can add ## additional info yourself. I just feel like the template PR should be more about what sections are required, than what sections are possible.
We require people to add tests, to explain what a PR does and why it does. But having a forced section for additional info looks meh to me. Does that make sense ?
There was a problem hiding this comment.
No, my point was that people don't necessarily add it themselves. If there's a placeholder for it, it's much more likely to be used. You could use your argument for the two remaining headlines in your PR in which case we should just delete the template all together.
There was a problem hiding this comment.
You could use your argument for the two remaining headlines in your PR in which case we should just delete the template all together.
No because those we actually require for every PR.
If there's a placeholder for it, it's much more likely to be used.
Granted. The only remaining question is what's the expected return of keeping it to exhort authors to use it, vs the annoyance of people having to remove it for all the small PRs that really don't need it. I think the expected return isn't that much, considering we don't have that many PRs from outsiders, and by definition small PRs where it's not needed should be more common than big PRs where it is. Buuuut I won't die on that hill so i'll let you decide. You may add a commit to add the section back if you think it's best. I don't mind 👍
There was a problem hiding this comment.
I question how big of an annoyance this actually is. And I think the benefits of having it outweighs the stated downsides. Especially in a world where AI is starting to make the first draft of a lot of these PR descriptions
There was a problem hiding this comment.
Especially in a world where AI is starting to make the first draft of a lot of these PR descriptions
I am an agent of the resistance, and you can be too! One must try to live, instead of passively accepting his fate.
More seriously, go ahead and add it back if you're convinced. I'm not convinced, but I can live with it 😄
I'm using the lazy-check technique here: if you're too lazy to add the commit back, then it's probably not that important to add it back hehe
There was a problem hiding this comment.
This section is actively being used by people in many PRs every week. I don't understand why that's not useful then - a lot of people seem to think it is. The AGENTS.md file being added here is being instructed to look at the PR template when authoring PR bodies.
There was a problem hiding this comment.
I'm still not convinced. I think it's unnecessary hand-holding. If a PR should have more informations, just add more informations, nothing stopping you from doing it. Unlike the other two sections, additional informations are optional. And this is why I think it's superfluous, even if I understand the argument about inviting authors to use it. Also, about your AGENTS.md argument, I'm not one to enjoy sacrificing human experience just because a fancy autocorrect is too stupid to do a good job.
Now I'll repeat what we can do:
- you can add it back by making a commit on this PR, I won't object (if you don't wanna take the time to do it, it's probably not a very important section then).
- Find other people that support your point, and I will fold to consensus, potentially in a guild meeting or something
| - [ ] Unit tests. | ||
| - [ ] Integration tests. | ||
| - [ ] Benchmarks. | ||
| - [ ] TypeScript [definitions][2]. | ||
| - [ ] TypeScript [tests][3]. | ||
| - [ ] API [documentation][4]. | ||
| - [ ] CI [jobs/workflows][5]. |
There was a problem hiding this comment.
Why the [ ]? This is no longer an issue/PR
There was a problem hiding this comment.
so people that want to use the checklist can copy paste it in their description or somewhere else ;)
There was a problem hiding this comment.
If it's meant to be copied it should be a code block, otherwise it's not possible. In its current state, it just shows disabled checkboxes that can't be checked
There was a problem hiding this comment.
It's a compromise between making it look good in markdown, and making the original copyable. Either you're reading the markdown in an editor and the original copyable is there, or you're on github interface and there is literally a button to switch to the original:
There was a problem hiding this comment.
I question the usefulness of this and would probably vote to just remove this section altogether
There was a problem hiding this comment.
I'm fine with removing it. It was a request from @bengl on my original slack thread.
There was a problem hiding this comment.
I don't think he intended for people to click "edit source" to actually use it 😉
There was a problem hiding this comment.
i'll have to let him say what he intended himself, don't wanna put words into his mouth. Although, once again it's not clicking "edit source", it's clicking the file then the tab "code", which is available for all markdown files to see the original code instead of the markdown render.
What does this PR do?
This removes the "plugin" checklist from the PR description template, as no one really uses it much, and it's a relic of when dd-trace-js was mostly just plugins.