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

Add PR template encouraging inline comments over normal comments #3717

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

Noratrieb
Copy link
Member

@Noratrieb Noratrieb commented Oct 24, 2024

RFC threads are known for turning into huge messes once many comments arrive. One among many reasons for this is that the comments are entirely disorganized, which makes it very hard to keep track.

I propose adding a warning the PR template that will strongly encourage people to leave inline review comments. These are significantly easier to keep track of (and can be resolved, hiding them!).

One question: who can/will approve this change? I'm just gonna go off vibes here, telling a bunch of teams about it and hoping that enough people like it that we can just ship it in and improve everyone's life!

Example (do feel free to comment normally here :D):

Important

When responding to RFCs, try to use inline review comments (it is possible to leave an inline review comment for the entire file at the top) instead of direct comments for normal comments and keep normal comments for procedural matters like starting FCPs.

This keeps the discussion more organized.

RFC threads are known for turning into huge messes once many comments arrive.
One among many reasons for this is that the comments are entirely disorganized, which makes it very hard to keep track.

I propose adding a warning the PR template that will strongly encourage people to leave inline review comments.
These are significantly easier to keep track of (and can be resolved, hiding them!).
@Noratrieb Noratrieb added the not-rfc For PRs that fix things like spelling mistakes, wrong file names, etc. label Oct 24, 2024
@Noratrieb
Copy link
Member Author

@Lokathor
Copy link
Contributor

Lokathor commented Oct 24, 2024

I find inline comments harder enough to fiddle with that I deliberately just avoid them entirely.

Particularly, I don't really read the source view of an RFC, I read the rendered view and hit "back" and then there's a box to say stuff about the RFC.

Usually whatever I want to say isn't about a specific line of the text, it's about something not in the text or it's about the entire direction of th text as a whole.

Also, and this is a minor thing but it's a thing, inline comments have worse preview text when you get the notification about them.

Copy link
Member

Choose a reason for hiding this comment

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

Usually whatever I want to say isn't about a specific line of the text, it's about something not in the text or it's about the entire direction of th text as a whole.

@Lokathor Comments can be added to a file directly.

Also, and this is a minor thing but it's a thing, inline comments have worse preview text when you get the notification about them.

Agree though in general notification previews on GitHub are terrible 🤢

Copy link
Member Author

Choose a reason for hiding this comment

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

I added a sentence about it

@joshtriplett
Copy link
Member

  1. I love this idea, and I'd love to see this change made.

  2. Inspired by this: next time we talk with GitHub, can we make the request of having a repository where all comments in general are always in "inline" form and create threads, and support "resolve"? Effectively, make every single comment on the PR always an inline comment, even if it's not attached to a particular line or file?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
not-rfc For PRs that fix things like spelling mistakes, wrong file names, etc.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants