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

Updating issue and PR templates #3132

Merged
merged 19 commits into from
Mar 19, 2020
Merged

Updating issue and PR templates #3132

merged 19 commits into from
Mar 19, 2020

Conversation

Poolitzer
Copy link
Member

I took drastic inspiration on the templates me and a friend wrote over here. No need to reinvent the wheel am I right. Give me feedback about them, I do have a certain... stlye :)

closes #3131 and a persistent voice in my head.

@gkeegan
Copy link
Contributor

gkeegan commented Feb 23, 2020

How does having two files under ISSUE_TEMPLATE work? Because doesn't GitHub usually take the one and make it a default when opening an issue?

Also, I think having bug and feature request separate can be problematic sometimes, since a lot of feature requests come in the form of people thinking it is a bug.

There is also the issue that this does seem quite lengthy and uses lots of markdown features that people might not be familiar with. There also is the problem that most of the time people don't properly use the templates anyways, or just ignore them. I don't know if making the template longer will help the problem or make it worse.

@Poolitzer
Copy link
Member Author

Poolitzer commented Feb 23, 2020

Thanks for the reply. Let me take it one by one.

How does having two files under ISSUE_TEMPLATE work? Because doesn't GitHub usually take the one and make it a default when opening an issue?

No, thats the old way. The new way is using this folder and with that we have the possibility of doing several templates. For a new issue, it will look a little something like this:

image

Also, I think having bug and feature request separate can be problematic sometimes, since a lot of feature requests come in the form of people thinking it is a bug.

While its true that feature requests may result from bugs or vice versa, there are still a lot of dedicated feature requests and a lot of dedicated bug reports. Manual review is needed anyway, templates are just a helping tool, getting a bit of structure in the madness.

There is also the issue that this does seem quite lengthy and uses lots of markdown features that people might not be familiar with. There also is the problem that most of the time people don't properly use the templates anyways, or just ignore them. I don't know if making the template longer will help the problem or make it worse.

Yes, ofc markdown is not used by everybody. But, from what I have seen in my other bigger repository, the template helped more then we thought. It makes it actually easier, because user understand what to replace and what to leave, At least my experience. Definitely did not make the situation worse. How do you see that happening?

@gkeegan
Copy link
Contributor

gkeegan commented Feb 23, 2020

For a new issue, it will look a little something like this:

Oh wow that looks nice. Good change.

still a lot of dedicated feature requests and a lot of dedicated bug reports.

Fair point. The specificness does out weight the negative.

How do you see that happening?

If people just open the issue and see a big wall of text, they may be less likely to read it properly than if it is small. There's no easy way to prove it I suppose.

I appreciate the effort you've put in and what you're trying to do, but I personally think it is a too much text overall and relies too much on people actually following the guidelines and using the right version of NewPipe. (It's obvious how many people don't read the templates whenever the video error changes happen and dozens of issues are created even after a new release has been made)

Copy link
Member

@B0pol B0pol left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I left my thoughts on this, overall it's great job.

@gkeegan , I don't think it's a problem because even if they don't know markdown, there is a "preview" button, and most importantly, people will follow the template, so even if they don't really use markdown, it will still be organised with the sections "steps to reproduce", "logs"…

.github/ISSUE_TEMPLATE/bug_report.md Outdated Show resolved Hide resolved
.github/ISSUE_TEMPLATE/feature_request.md Outdated Show resolved Hide resolved
.github/PULL_REQUEST_TEMPLATE.md Outdated Show resolved Hide resolved
.github/PULL_REQUEST_TEMPLATE.md Show resolved Hide resolved
.github/ISSUE_TEMPLATE/bug_report.md Show resolved Hide resolved
.github/ISSUE_TEMPLATE/bug_report.md Show resolved Hide resolved
.github/ISSUE_TEMPLATE/bug_report.md Outdated Show resolved Hide resolved

#### Additional context
Add any other context or screenshots about the feature request here.
Ex. *Here's a photo of my cat!*
Copy link
Member

@B0pol B0pol Feb 23, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We are at the bottom of the template, and I disagree about "a clear and concise description" everywhere.

IMO, at least one paragraph should say:
Why you'd really want this feature? Explain how it will change your life and your NewPipe experience!

This was just an example, but the idea is to have one paragraph where people THINK about their request. Do they really WANT it, or it was just a thought they had but they don't really care about it.

And if they give good reasons why they really WANT it, we can understand better, and prioritize (dunno if it's how we say) these requests, or on the contrary, if they didn't give good reasons AND it seems like a niche request, maintaners / contributors will know there are more important things to fix / features to bring.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and I disagree about "a clear and concise description" everywhere.

You would delete this sentence everywhere?

Why you'd really want this feature? Explain how it will change your life and your NewPipe experience!

MY DEAD MOTHER COULD COME BACK! yeah I get you good point

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You would delete this sentence everywhere?

No, I didn't meant to remove it everywhere. My bad ih you understood that

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

okay, uhm. how can I understand that part then

.github/PULL_REQUEST_TEMPLATE.md Outdated Show resolved Hide resolved
.github/PULL_REQUEST_TEMPLATE.md Outdated Show resolved Hide resolved
.github/PULL_REQUEST_TEMPLATE.md Outdated Show resolved Hide resolved
.github/ISSUE_TEMPLATE/bug_report.md Outdated Show resolved Hide resolved
@Poolitzer
Copy link
Member Author

I appreciate the effort you've put in and what you're trying to do, but I personally think it is a too much text overall and relies too much on people actually following the guidelines and using the right version of NewPipe. (It's obvious how many people don't read the templates whenever the video error changes happen and dozens of issues are created even after a new release has been made)

Not much I can do about your personal believes. I'd still say lets try it out. Nothing can go wrong right.

@gkeegan
Copy link
Contributor

gkeegan commented Feb 24, 2020

I'd still say lets try it out. Nothing can go wrong right.

True. If some people aren't going to use templates anyways then at least the rest will be following a standard that is informative.

@Poolitzer
Copy link
Member Author

Exactly. And the people not following the new ones didnt follow the old one either.

@gkeegan
Copy link
Contributor

gkeegan commented Feb 24, 2020

Yeah. I'll suggest some improvements for this at some points tomorrow. I have some written up but am away from my computer at the moment.

@Poolitzer
Copy link
Member Author

Looking forward to it.

Copy link
Member

@B0pol B0pol left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

image
image

It seems like you wanted a newline, but it didn't do it. In raw Markdown, it's two spaces to add a newline, or two newlines for a new paragraph.
You can preview with « display the rich diff ».

Last thing, while watching /e/ OS' templates for new issues (you can't open new issue without an /e/ gitlab account), I found the summary, and it could be a nice addition to our template.

Summary

  • The device is unusable
  • The bug is the source of a data loss or a big waste of time
  • The bug concerns a third party app
  • The bug concerns security
  • The bug concerns privacy

/e/ is a custom rom so we'd have to adapt the summary to NewPipe but I really like this idea, don't you?

.github/ISSUE_TEMPLATE/bug_report.md Outdated Show resolved Hide resolved
.github/ISSUE_TEMPLATE/bug_report.md Outdated Show resolved Hide resolved
.github/ISSUE_TEMPLATE/bug_report.md Outdated Show resolved Hide resolved
.github/ISSUE_TEMPLATE/feature_request.md Outdated Show resolved Hide resolved
@Poolitzer
Copy link
Member Author

I will have a look, looks good to me

@Poolitzer
Copy link
Member Author

Poolitzer commented Feb 24, 2020

It seems like you wanted a newline, but it didn't do it.

Uhm, regarding this: No, I just wanted to separate the headings from each other. If you create a test issue somewhere and hit preview, you will see it works. At least the way I imagine it

Last thing, while watching /e/ OS' templates for new issues

I tried to open the link but it isnt valid

Edit: fixed it, nevermind, not really helping. So lets brainstorm. What would you guys like in a checkbox made summary

@TobiGr TobiGr added the meta Related to the project but not strictly to code label Feb 26, 2020
Copy link
Contributor

@TobiGr TobiGr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That loooks good. Thank you!

.github/PULL_REQUEST_TEMPLATE.md Outdated Show resolved Hide resolved
.github/ISSUE_TEMPLATE/bug_report.md Outdated Show resolved Hide resolved
.github/PULL_REQUEST_TEMPLATE.md Show resolved Hide resolved
And its our app!

Co-Authored-By: Tobias Groza <[email protected]>
@B0pol
Copy link
Member

B0pol commented Feb 27, 2020

I've noticed one thing is missing in issues / bug report templates: NewPipe version
We could do like before, a checkbox, or somewhere in the template where we have to write our NewPipe version.

@TobiGr
Copy link
Contributor

TobiGr commented Feb 27, 2020

@B0pol is correct. People should fill in the version.

@Poolitzer
Copy link
Member Author

Agreed. Will add it.

TobiGr
TobiGr previously approved these changes Mar 1, 2020
Copy link
Contributor

@TobiGr TobiGr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me now. I'll leave this open until tomorrow and merge unless somebody wants to see more changes.

@Poolitzer
Copy link
Member Author

I will just quickly bring the branch up to date then

because GitHub thought its funny to hide it for a reason.

Co-Authored-By: opusforlife2 <[email protected]>
@Poolitzer
Copy link
Member Author

I am getting offtopic and people will get angry, but jkdev at stackoverflow agrees with me.

Co-Authored-By: opusforlife2 <[email protected]>
@opusforlife2
Copy link
Collaborator

I am getting offtopic and people will get angry, but jkdev at stackoverflow agrees with me.

Interesting. This is a topic where we don't seem to have settled on a convention yet.

@Poolitzer
Copy link
Member Author

What we can settle on (and even more offtopic oh god): This converstion is under the top 10 longest one we have in this project: https://github.com/TeamNewPipe/NewPipe/issues?utf8=%E2%9C%93&q=sort%3Acomments-desc+comments%3A%3E50. Had to add the over 50 mark, otherwise too many issues mixed up the order

@wb9688
Copy link
Contributor

wb9688 commented Mar 3, 2020

What I'd add is an explanation of how to check a checkbox, as you could see how many people don't understand that on the current issues.

@opusforlife2
Copy link
Collaborator

What I'd add is an explanation of how to check a checkbox, as you could see how many people don't understand that on the current issues.

What uBlock Origin has them do is create the issue, then mouse click the check boxes to tick them. I think that's easier than if we were to try to explain Markdown or even tell them to "type in an x and ensure there are no spaces between that and the brackets."

.github/ISSUE_TEMPLATE/bug_report.md Outdated Show resolved Hide resolved
.github/ISSUE_TEMPLATE/bug_report.md Outdated Show resolved Hide resolved
.github/PULL_REQUEST_TEMPLATE.md Show resolved Hide resolved
@TobiGr
Copy link
Contributor

TobiGr commented Mar 4, 2020

I am not sure if this is the right place to do this, but maybe we can also add to the PR template that people should rebase their commits instead of creating tons of merge commits.

@opusforlife2
Copy link
Collaborator

That's a valid request. It should be added.

@Poolitzer
Copy link
Member Author

sighs okay. everybody gets one more edit request and after that Im gonna throw stuff at them

@Poolitzer
Copy link
Member Author

@TobiGr why does it matter? Dont you squash the commits in the end?

@TheAssassin
Copy link
Member

TheAssassin commented Mar 4, 2020

@Poolitzer squashing PRs makes sense in extremely rare cases only. Mostly when it's a set of maybe, 15 commits or so, which only change the same line, and would've fit better as one commit. Normally, you want to keep the history of changes. When you have to investigate a bug and need to read the history, you need to be able to see what has changed when and especially why (remember folks, commit messages are not about the what (the reader usually understands the code), but the why; same goes for comments).

Discussions on GitHub or any other platform are not as persistent as code history. One might move away from one platform to another. Heck, it might just die today. The repository will continue to live on, though. There's hundreds of copies which we could make into one repository somewhere else again. Referencing them is only useful temporarily, e.g., when you want to create a notification or link in an issue or PR. In a year or two, nobody will dig out the old PR's comments.

A milder approach for getting a history which makes more sense is to rewrite the Git history. That is a skill many experienced Git users have learned. This way, a history can be flattened and changes can be sorted so they make sense retrospectively. It's usually preferred over squashing, which is like hitting a tiny nail with a 20 pounds hammer.

The more experienced you are, the more reluctant you get to using some "squash & merge" stuff... usually because you experienced the disadvantages before, trying to sort out some mess...

Edit: @TobiGr suggests a "do it right from the beginning" solution, avoiding messy merges for rebase (where possible). I'm just not sure whether a PR template is the right place for such a hint. It's usually too late then, isn't it? Perhaps that should go into some contribution guidelines?

@Poolitzer
Copy link
Member Author

Poolitzer commented Mar 4, 2020

Okay assassin, got some good points there. We have a different approach in our other repo, but never experienced any setbacks from it; its arguably way smaller though.

I think you are right with the contribution, it feels more appropriate in Contributing then in those templates.

Copy link
Contributor

@TobiGr TobiGr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me now.

Just one small thing.

.github/ISSUE_TEMPLATE/bug_report.md Outdated Show resolved Hide resolved
.github/ISSUE_TEMPLATE/feature_request.md Outdated Show resolved Hide resolved
Copy link
Contributor

@TobiGr TobiGr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you again!

@Stypox Stypox self-requested a review March 19, 2020 17:12
@TobiGr TobiGr merged commit cab104a into TeamNewPipe:dev Mar 19, 2020
This was referenced Mar 25, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
meta Related to the project but not strictly to code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Issue templates
8 participants