express
express copied to clipboard
doc: add section for project captains
Refs: https://github.com/expressjs/discussions/issues/98 Refs: https://github.com/expressjs/discussions/issues/106#issuecomment-595342919 co-authored-by: Wes Todd @wesleytodd
@dougwilson , @wesleytodd : we are ready to land this, isn't it? what is blocking this?
@dougwilson / @wesleytodd - can you either remove the red-X or clarify what your concern is? If it is just that one is waiting for other's approval, please state so, so that we can move forward.
/cc @expressjs/express-tc - FYI, and important PR for defining project captains for repos. Please chime in if you have comments, or else complete review to help progress!
There is a simple issue here I was going to fix on merge, but didn't get a chance to this weekend with what is going on in my country. After work today I will at least comment on it so anyone can make the change if I cannot.
@dougwilson - I have addressed all the review comments, mostly following your suggestions AS IS
. Please have a look and do the needful!
it has been 11 days since the last update. can we take this forward?
@dougwilson - I just squashed all the commits into one
Thanks @gireeshpunathil ! As we discussed on the TC meeting, it should be in the format as the current commit on master
and our guide: the message on the first line, a blank line, a reference to the PR in the form "closes #PR
". I can make the said changes if you need. LMK
@dougwilson - made the changes to the commit message as per the guidelines; PTAL.
Ok... It still does not seem to be in the form from my comment... ? I can just update if you need, let me know. If I can better clarify how the message should be constructed let me know as well. I believe it was clear, and provided an example of a previous comment message, but if I can provide better guidance for the format, I certainly can.
I am circling around on this because I would like this to get merged, but I recall that you wanted to do the commit editing so I could just do the fast-forward merge, but it's not in the correct state for that. I can make those edits if you like, but don't want to force push on your branch without you being aware as per our previous conversation :)
@dougwilson -
- thanks - yes, I prefer to be advised over edits if possible, towards improving my own knowledge on those areas.
this is the current commit message:
doc: add section for project captains
closes https://github.com/expressjs/discussions/issues/98
Co-Authored-By: Wes Todd <[email protected]>
this is the rule:
the message on the first line, a blank line, a reference to the PR in the form "closes #PR".
- the message is on the first line
- there is a blank line
- there is a reference to the PR against
closes
verb
the two additional things are:
- there is a subsystem (doc) in the message line (following the existing conventions)
- there is a co-author field (attribution of authorship, many of the original wordings came from Wes)
please advise where is the change required?
- The subsystem is "docs" and not "doc"
- The closes should be
closes #4210
- We do not include co-author fields, but we can if desired to amend our guidelines, in which this commit would wait for that to happen
It should just be something like
docs: add project captains to contribution
closes #4210
Then as discussed in the TC meeting, the committer on the commit would need to be the person performing the merge itself (myself unless you would like to wait for another to do that).
@dougwilson - adjusted the commit message accordingly, PTAL!
@wesleytodd - I had earlier kept you as a co-author
for this PR, but looks like we do not include that as per the current process. Can you please review and provide your concurrence? If you have concerns pls let me know, we would need to go the route of amending guidelines.
So based on the conversation going on in https://github.com/expressjs/discussions/issues/121 it seems like this is not ready to land as-is, as it seems that there may be either additional discussion to have on the goal here and/or updating to the wording in this pull request to reflect it. Specifically in regards to what falls under repo ownership.
It would come to reason to me that landing pull requests would be a "day-to-day" task for said project captains, and their particular merging strategies, commit message formats, etc. would serve the repo on a technical front, as outlined in this document.
I think part of the goal of project captains is to empower them to manage their repos, and commit messages seems to fall directly in that category to me. For example, if a project captain wants to use something like standard-release to automate releases and history file generation, they may need specific structure to commit messages. We should strive to empower the owners to do what they think will improve their workflow.
But maybe I'm misunderstanding on that point. Especially if I'm misunderstanding, that definately means we need greater clarity in the document, as we don't want to have everyone interpreting it differently, I presume. I'm happy to provide suggested edits on the wording, but likely need input from the original author @wesleytodd to sync on the interpretation so I can help clarify 👍
@dougwilson - it seems strange (and funny) to me that you closed https://github.com/expressjs/discussions/issues/121 with no action on it, and then quoting its content as a reason for further work here!
Hi @gireeshpunathil that issue is orthogonal to this issue.
But maybe I'm misunderstanding on that point. Especially if I'm misunderstanding, that definately means we need greater clarity in the document, as we don't want to have everyone interpreting it differently, I presume.
here is the sequence of events, for better clarity for everyone:
- the commit message did not follow this repo's convention
- more specifically, I added
Co-authored-by
field, but not not an established practice in this repo - @dougwilson pointed out that, I promptly removed it, but sought verbal consent from the co-author
- I opened an issue to discuss that: specifically, implementing a guideline pan-project
- that is closed as inappropriate in that repo, and to be dealt in individual repos.
- now back here, @dougwilson says it is causing misunderstanding!
This is becoming a surprise to me! where is the mis-understanding? If you assert that no org wide guidelines are present, so be it, and I will seek change proposal with captains, period! where is it that I interpreted this document at all? leave alone interpreted differently?
I suggest we take a step back;
- retire the discussion in https://github.com/expressjs/discussions/issues/121 as it is already closed and I agreeing to your proposal to take anything further with individual repo captians
- and go ahead with this proposal
Hi @gireeshpunathil please let me know if there is some time in which you are available for a voice chat; I would like to proceed with landing this PR, but I am honestly unsure at this point how to interact with your pull requests that will not come off in bad faith, and would like a chance to have a discussion with you so we are on the same page so we can make progress landing.
@dougwilson - I sent you a mail with my time preferences.
in general, this can have more reviews!
I don't have any real comments on this PR, but once the discussion is resolved and it lands, we should make sure the change is reflected on the website; I opened https://github.com/expressjs/expressjs.com/issues/1184 to track that.
I believe I have addressed all the outstanding comments. pinging @expressjs/express-tc , to seek some reviews or take it towards landing.
I am locking this PR until the above issue with the interaction between myself and @gireeshpunathil is resolved.
Closing in favor of #5484.