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

docs: add a triage section #4055

Merged
merged 1 commit into from
May 11, 2020
Merged

Conversation

gireeshpunathil
Copy link
Contributor

ref: expressjs/discussions#92

thanks in advance!

@helio-frota
Copy link

Looking good and clear.
👍

Contributing.md Outdated Show resolved Hide resolved
Contributing.md Outdated Show resolved Hide resolved
Copy link

@samkamer samkamer left a comment

Choose a reason for hiding this comment

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

Suggested changes to improve general readability, especially for newbie contributors.

Contributing.md Outdated Show resolved Hide resolved
Contributing.md Outdated Show resolved Hide resolved
Contributing.md Outdated Show resolved Hide resolved
Contributing.md Outdated Show resolved Hide resolved
Contributing.md Outdated Show resolved Hide resolved
Contributing.md Outdated Show resolved Hide resolved
Contributing.md Outdated Show resolved Hide resolved
Copy link

@samkamer samkamer left a comment

Choose a reason for hiding this comment

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

Thanks @gireeshpunathil for the quick turnaround on the suggestions.

Contributing.md Outdated Show resolved Hide resolved
Contributing.md Outdated Show resolved Hide resolved
Contributing.md Outdated Show resolved Hide resolved
Contributing.md Outdated Show resolved Hide resolved
Contributing.md Outdated Show resolved Hide resolved
Copy link
Member

@wesleytodd wesleytodd 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! I think we might need to create all of these tags in the some of the repos and then we will have to get the automation working to streamline this, but we can follow with that later. If @dougwilson also approves then it looks like this is good to merge!

@gireeshpunathil
Copy link
Contributor Author

@wesleytodd - per earlier discussion, I also added a two-liner describing steps to become a triager, and linking the example to it, as a reference. PTAL!

Copy link
Contributor

@dougwilson dougwilson left a comment

Choose a reason for hiding this comment

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

Shouldn't we add something to the vocabulary section to define what a triager is like the other definitions as well? @wesleytodd

@dougwilson
Copy link
Contributor

I pushed a commit taking a stab at adding to the vocab if someone wants to take a look :)

@dougwilson
Copy link
Contributor

I'm working to fix the inconsistent line wrapping length, capitalization, and punctuation to help clean it up 👍

@wesleytodd
Copy link
Member

Hey! I am back from vacation so am going to work today on wrapping up all these triage requests if I can. @dougwilson I am going to to a full pass at cleaning up the formatting and wording before merging this, but are there any other large outstanding changes you want to see?

@dougwilson
Copy link
Contributor

Besides still just needs a pass through to fix grammar like punctuation and capitalizing, there are sentences that feel weird to read. I'm sure you will see as you read through it. Also the "Other Helpful Hints" reads like a "brain dump" than a cohesive document.

Lastly, the becoming section should be self-contained instructions vs referring to a pr. That way when something changes for the instructions it is easy to make the change in the doc itself.

@dougwilson
Copy link
Contributor

Another thought I had is that we have a doc Release-Process.md and so I wonder if a lot of this should actually be in a Triage-Process.md or not. I'm not really set on either way, just it crossed my mind and maybe if I wrote it here you may have a strong opinion:)

@wesleytodd
Copy link
Member

Great! That was basically what I was thinking. I am merging the request PRs now and will do this one last and then make a final PR with the whole traige branch. It might take a little bit because many of these PR's are messy because of the swapped branch and force push, but I should be able to have all this done today!

@wesleytodd
Copy link
Member

wonder if a lot of this should actually be in a Triage-Process.md

Oh, I kind of like that idea! Once I get it cleaned up I will see if we can easily move it all out.

@dougwilson
Copy link
Contributor

Nice. I have just been updating this PR directly with my changes so far.

@wesleytodd wesleytodd changed the base branch from master to triage December 31, 2019 19:22
@wesleytodd
Copy link
Member

wesleytodd commented Dec 31, 2019

I have never actually tried before, but when I try to push to the PR branch it doesn't seem to let me:

$ git remote add gireeshpunathil git://github.com/gireeshpunathil/express.git
$ git push gireeshpunathil gireeshpunathil-triage-docs:triage
fatal: remote error:
  You can't push to git://github.com/gireeshpunathil/express.git
  Use https://github.com/gireeshpunathil/express.git

If I try with the https remote it just asks for my username/password then fails saying I am not allowed. Is there some special permission I am missing? Also to note, I rebased this ontop of the existing triage branch, so I do need to force push, is that the issue?

@jonchurch
Copy link
Member

jonchurch commented Jan 23, 2020

If @gireeshpunathil hasn't already, enabling contributors to push to his PR branch should allow you @wesleytodd to push changes, I think you (wes) need write perms on this repo though for that to work.

Or you can start a PR against this branch at his fork.

@helio-frota
Copy link

Hi,

What if a triager get requested for a pull request review ?

Probably I was assigned to review some because I was the last one to be added as triager, maybe github automatically suggested me to be selected for the review (?)

#4166 and #4156

@jonchurch
Copy link
Member

Just wanted to note that the language here mentions a needs triage label which I don't believe exists yet.

I also would advocate for, in the future, a bot that adds that to all new unlabeled issues.

@dougwilson dougwilson changed the base branch from triage to master May 11, 2020 04:47
@dougwilson dougwilson dismissed their stale review May 11, 2020 04:49

addressed

@dougwilson dougwilson merged commit 18da651 into expressjs:master May 11, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants