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

doc: Reorder collaborators by their usernames #2322

Closed
wants to merge 1 commit into from

Conversation

jbergstroem
Copy link
Member

Followup from #1966 where we didn't quite agree on TC ordering. Now that we've resolved that, I'm suggesting we sort by github usernames.

(hopefully I got my a-b-c's straight)

Fixes #1972.

@jbergstroem jbergstroem added the doc Issues and PRs related to the documentations. label Aug 7, 2015
@bnoordhuis
Copy link
Member

Rubber-stamp LGTM.

I didn't notice it before but the convention is to use all lowercase in the first line of the commit log, i.e. s/Reorder/reorder/.

@targos
Copy link
Member

targos commented Aug 7, 2015

Would it make sense to order by GH username but keep the real name at the beginning ?
The rendering is weird: https://github.com/jbergstroem/io.js/blob/doc/author-order-r2/README.md#tsc-technical-steering-committee

@jbergstroem
Copy link
Member Author

@bnoordhuis I like that. Been pondering this myself as well. Thanks.

@targos what do you mean by 'weird'? With my suggested change, I like how its an easy copy paste into a Reviewed-By as well as finding people by their usernames (read: navigating github).

@targos
Copy link
Member

targos commented Aug 7, 2015

@jbergstroem You can do the same in both versions.

Current readme:

Your version:

I personally prefer current.

@targos
Copy link
Member

targos commented Aug 7, 2015

I realize that if the username stays at the end, it's not easy to find it visually.
The weird part is that each line starts with @. You could perhaps just remove this character ?

@jbergstroem
Copy link
Member Author

I'm happy to remove it [the prefixed @] if others agree.

@thefourtheye
Copy link
Contributor

I am okay with removing @. Also can we mention that the names are in alphabetical order?

@jbergstroem
Copy link
Member Author

@thefourtheye I thought about that, but it felt silly - similar to us adding a note about the current order is based on when they joined?

@thefourtheye
Copy link
Contributor

Okay no problem :)

@jbergstroem
Copy link
Member Author

@thefourtheye better?

Perhaps someone else of the @nodejs/collaborators wants to chip in?

Also: moving into flavour here, but I'd be happy to drop wrapping names in bold.

@thefourtheye
Copy link
Contributor

@jbergstroem LGTM

@trevnorris
Copy link
Contributor

The Reviewed-By argument is not very applicable now that we've also decided to move forward with automated patch landing. Other than that, what's the reason for ordering based on github handle?

@jbergstroem
Copy link
Member Author

@trevnorris good point -- the only other reason I could think of would be avoiding sorting by a-z when we for instance have cyrillic in there.

@trevnorris
Copy link
Contributor

@jbergstroem Sure thing. Even if there wasn't an argument for alphabetically ordering the names the change doesn't bother me. Just wanted to make sure everyone realized that point. :)

Or... We could use a randomization algorithm and have it list everyone...

@orangemocha
Copy link
Contributor

+1

@jbergstroem
Copy link
Member Author

ok, last stab; now follows the newly implemented 'reviewed-by' style:
$username - $name $surname <email@address.com>

@orangemocha
Copy link
Contributor

LGTM

@orangemocha
Copy link
Contributor

This can be landed in Jenkins as NODES_SUBSET pure_docs_change, skipping tests.

@thefourtheye
Copy link
Contributor

LGTM

jbergstroem added a commit that referenced this pull request Sep 3, 2015
Fixes: #1972
PR-URL: #2322
Reviewed-By: orangemocha - Alexis Campailla <orangemocha@nodejs.org>
Reviewed-By: thefourtheye - Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
@jbergstroem
Copy link
Member Author

Merged in 6ce8f5f. Thanks for feedback and review.

@jbergstroem jbergstroem closed this Sep 3, 2015
jbergstroem added a commit that referenced this pull request Sep 3, 2015
Fixes: #1972
PR-URL: #2322
Reviewed-By: orangemocha - Alexis Campailla <orangemocha@nodejs.org>
Reviewed-By: thefourtheye - Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc Issues and PRs related to the documentations.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Reorder authors in README.md
7 participants