Github codeowners/templates changes #1491
Conversation
lemunozm
requested review from
mikiquantum,
mustermeiszer,
branan and
NunoAlexandre
as code owners
.github/pull_request_template.md
Outdated
| # Checklist: | ||
|
|
||
| - [ ] I have added Rust doc comments to structs, enums, traits and functions | ||
| - [ ] I have made corresponding changes to the documentation | ||
| - [ ] I have performed a self-review of my code | ||
| - [ ] I have added tests that prove my fix is effective or that my feature works |
There was a problem hiding this comment.
I am not sure whether I can agree with this removal. Personally, I go through this checklist for every PR but I suppose the reality is, that it is rather ignored and many PRs do not include any ticks.
There was a problem hiding this comment.
Same here, I like having this checklist for chores that might be forgotten.
There was a problem hiding this comment.
Yes, In some PRs, I also found myself reading it to check mentally if things were done.
But, I found 2 points against having this there (maybe some other in place?).
- Most of the time, some points do not apply to the PR meaning.
- Most of the time, PRs are merged without clicking done in all those points.
From my point of view, I think each PR has its own checklist and should be thought of specifically for the PR purpose. So I agree with adding a checklist, but I think should be done explicitly at that moment of creation for the content of that PR.
.github/issue_template.md
Outdated
| [Description of the issue or feature] | ||
|
|
||
| ### Research/based on | ||
|
|
||
| [Which codebases, pallets or designs did you base your design on] | ||
|
|
||
| ### How will this affect the code base | ||
|
|
||
| [Does it improve readability, performance? Will it introduce new complexity?] | ||
|
|
||
| ### What are foreseen obstacles or hurdles to overcome? | ||
|
|
||
| [Are migrations needed, structural changes around testing etc?] |
There was a problem hiding this comment.
I like that sections TBH and found it helpful to have the structure.
There was a problem hiding this comment.
my POV is that these are often good questions to ask oneself when creating an issue but I personally don't find value is having to try and fit this structure per se. my 2cents, whatever is more helpful to the team is good with me 👍
There was a problem hiding this comment.
IMO a section for description and probably another one for extra notes might be more than enough.
There was a problem hiding this comment.
I am good with what the matority wants
There was a problem hiding this comment.
I do not have a strong opinion of these sections. It's true that sometimes they do not apply to the issue or can be handled by tags (as "migration needed"), but they also help to explain the issue better
There was a problem hiding this comment.
Personally, I would like to keep this but it seems like the majority choses to ignore it. I don't have a strong opinion.
There was a problem hiding this comment.
@wischli maybe we can find a middle ground that makes everyone happy?
|
I knew it would open discussions 😆 |
|
I'll leave this PR open until/during next week, to get a chance for others to give arguments |
|
I think, given most of you want to keep these template parts, I'm going to leave it as it is. If in some months you find it less usable, then can remove it. Is everyone ok with it? |
|
@wischli @NunoAlexandre @cdamian @mustermeiszer would you want to add yourself to the CODEOWNERS file with the new liquidity-pools-related content we added last month? Feel free to add a commit in this branch |
cdamian
dismissed stale reviews from mustermeiszer and NunoAlexandre
via
c96cd00
cdamian
force-pushed
the
github-changes
branch
from
9c9f904
to
c96cd00
Compare
lemunozm
force-pushed
the
github-changes
branch
from
c96cd00
to
88a1d33
Compare
Changes and Descriptions
*.tomlmatching to avoid having all of us in all PRs.