express icon indicating copy to clipboard operation
express copied to clipboard

doc: add section for project captains

Open gireeshpunathil opened this issue 4 years ago • 22 comments

Refs: https://github.com/expressjs/discussions/issues/98 Refs: https://github.com/expressjs/discussions/issues/106#issuecomment-595342919 co-authored-by: Wes Todd @wesleytodd

gireeshpunathil avatar Mar 09 '20 08:03 gireeshpunathil

@dougwilson , @wesleytodd : we are ready to land this, isn't it? what is blocking this?

gireeshpunathil avatar Mar 20 '20 06:03 gireeshpunathil

@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!

gireeshpunathil avatar Mar 23 '20 12:03 gireeshpunathil

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 avatar Mar 23 '20 13:03 dougwilson

@dougwilson - I have addressed all the review comments, mostly following your suggestions AS IS. Please have a look and do the needful!

gireeshpunathil avatar Mar 24 '20 03:03 gireeshpunathil

it has been 11 days since the last update. can we take this forward?

gireeshpunathil avatar Apr 04 '20 10:04 gireeshpunathil

@dougwilson - I just squashed all the commits into one

gireeshpunathil avatar Apr 09 '20 00:04 gireeshpunathil

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 avatar Apr 09 '20 00:04 dougwilson

@dougwilson - made the changes to the commit message as per the guidelines; PTAL.

gireeshpunathil avatar Apr 09 '20 00:04 gireeshpunathil

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 avatar Apr 15 '20 02:04 dougwilson

@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?

gireeshpunathil avatar Apr 15 '20 03:04 gireeshpunathil

  1. The subsystem is "docs" and not "doc"
  2. The closes should be closes #4210
  3. 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 avatar Apr 15 '20 03:04 dougwilson

@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.

gireeshpunathil avatar Apr 15 '20 03:04 gireeshpunathil

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 avatar Apr 15 '20 06:04 dougwilson

@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!

gireeshpunathil avatar Apr 15 '20 06:04 gireeshpunathil

Hi @gireeshpunathil that issue is orthogonal to this issue.

dougwilson avatar Apr 15 '20 06:04 dougwilson

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

gireeshpunathil avatar Apr 15 '20 07:04 gireeshpunathil

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 avatar Apr 17 '20 06:04 dougwilson

@dougwilson - I sent you a mail with my time preferences.

gireeshpunathil avatar Apr 17 '20 07:04 gireeshpunathil

in general, this can have more reviews!

gireeshpunathil avatar Jun 28 '20 15:06 gireeshpunathil

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.

crandmck avatar Jul 26 '20 20:07 crandmck

I believe I have addressed all the outstanding comments. pinging @expressjs/express-tc , to seek some reviews or take it towards landing.

gireeshpunathil avatar Jul 27 '20 02:07 gireeshpunathil

I am locking this PR until the above issue with the interaction between myself and @gireeshpunathil is resolved.

dougwilson avatar Jul 27 '20 02:07 dougwilson

Closing in favor of #5484.

wesleytodd avatar Feb 16 '24 15:02 wesleytodd