Contributing.md Enhancement #1683
Conversation
✅ Deploy Preview for expressjscom-preview ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
chrisdel101
requested review from
wesleytodd,
crandmck,
bjohansebas and
a team
|
@chrisdel101 let's try to only modify things related to contributing in this PR; I'll review this PR in detail over the weekend. |
chrisdel101
force-pushed
the
contributing
branch
3 times, most recently
from
4283961
to
44cb937
Compare
Done. Please run the pipeline |
- apply all branch changes
changes to soften workflow language
Add gh important tags to top of page
chrisdel101
force-pushed
the
contributing
branch
3 times, most recently
from
c6af18e
to
3ab360c
Compare
There was a problem hiding this comment.
@bjohansebas Were these (files are now missing- I'm referring to the commits ci: update...) part of this PR? I just reversed them since I thought they were mistakes, then looked again.
I guess you made these on this branch right?
I'll add them back (remove last two commits) in this was your intention.
EDIT: I added these back in.
EDIT2: Removing them was correct, so they are removed again.
chrisdel101
force-pushed
the
contributing
branch
2 times, most recently
from
c6c6266
to
e2f0788
Compare
|
btw, it's not necessary to run the workflow, running the script and making the commit in this PR is fine. The workflow is just a step to keep the rest of the documentation up to date. |
chrisdel101
force-pushed
the
contributing
branch
from
e2f0788
to
32196f9
Compare
|
@crandmck @wesleytodd @UlisesGascon Sebastian helped with all the technical stuff but could one of you read over the text content and let me know if you want any changes? |
- remove over use of important
- remove uz.css as it came back
- removing CI commits
- add HR line
-Add GH NOTE tag and filter out
chrisdel101
force-pushed
the
contributing
branch
from
f72825b
to
720563a
Compare
You mean the CI? How do I stop it from running? I ran the workflow action outisde this repo to make sure the script paths didn't fail, if this is what you meant. |
This comment explains it quite well (#1553 (comment)) |
CONTRIBUTING.md
Outdated
| If you want to learn about working on Expressjs.com, this is the right place. Follow the steps below to get started. | ||
|
|
||
| #### TL;DR | ||
| 1. Open an issue and get approval. |
There was a problem hiding this comment.
is approval necessary? who is delegated the authority to make these approvals?
There was a problem hiding this comment.
This should not be in here at all imo. The process as with any other repo in this org, is "if it is complicated and/or you dont want to waste your time on a PR that my not land, ask if it is a good idea, otherwise open a PR"
There was a problem hiding this comment.
I am going to copy over a quote from the discussion on this from the slack.
I think this all makes sense, and thanks for sharing the context it for sure helps. I would guess @crandmck
probably is on the same page (correct me if I am wrong) though that approval is not required but opening an issue to discuss is just helpful if you are new or don’t want to spend time opening a PR that might not land.
This is an OSS lesson I think all of us learned at one point or another, and I would LOVE to improve documentation on this to help others learn without the morale hit or wasted effort. But I just want to be clear, the language in the PR says it is required to get approval, which is not at all the case.
There was a problem hiding this comment.
I tried to remove any offensive language around this, and can change the wording again, but this idea was literally the point of redoing this document. Otherwise we might be good to just leave things as they are currently.
This was mentioned before I started this #1506 (comment).
@bjohansebas Did we not want this step in our process? In my observations, we have been following this process for the past few months at least.
Approval is informal and can be done by any member.
There was a problem hiding this comment.
Here is another instance where we used this. #1568 (comment) which resulted in #1590
Can we reword this idea somehow to make it more acceptable? Something like:
If you have any questions or concerns about the utility of your submission, consider opening an issue first to discuss with the team, etc
There was a problem hiding this comment.
How about something like this:
"You're strongly encouraged to open a discussion issue before starting work on a PR, particularly for significant changes that have wide impact. Small minor improvements or corrections don't need an issue, but for more pervasive contributions, an issue helps to clarify and focus the work and ensure it aligns with the overall project priorities."
There was a problem hiding this comment.
I've made some changes to the wording in those sections and removed the TL;DR part.
I tried to soften it, make it seem optional, remove all references to "approval", and and say it's for large changes only.
CONTRIBUTING.md
Outdated
| If you want to learn about working on Expressjs.com, this is the right place. Follow the steps below to get started. | ||
|
|
||
| #### TL;DR | ||
| 1. Open an issue and get approval. |
There was a problem hiding this comment.
This should not be in here at all imo. The process as with any other repo in this org, is "if it is complicated and/or you dont want to waste your time on a PR that my not land, ask if it is a good idea, otherwise open a PR"
CONTRIBUTING.md
Outdated
| If you want to learn about working on Expressjs.com, this is the right place. Follow the steps below to get started. | ||
|
|
||
| #### TL;DR | ||
| 1. Open an issue and get approval. |
There was a problem hiding this comment.
How about something like this:
"You're strongly encouraged to open a discussion issue before starting work on a PR, particularly for significant changes that have wide impact. Small minor improvements or corrections don't need an issue, but for more pervasive contributions, an issue helps to clarify and focus the work and ensure it aligns with the overall project priorities."
|
I commented inline, but we should be sure to clarify that no approval is necessary (i.e. in an issue) to start a PR. IMO an initial discussion issue is strongly recommended for very large contributions, just to ensure they make sense. The linked discussions came about because of very large refactoring PRs that had no previous discussion at all. While such PRs are not "prohibited", they can often lead to wasted time not only for the contributor but for the TC/approvers. External contributors may not always be aware of ongoing discussions or work and can open PRs that don't align with that and are very unlikely to land. Such PRs are certainly not prohibited, but are not a good use of anyone's time. |
- remove all "approval" language - add optional wording - make everything Express JS wording - remove italics as it not required - remove TLDR
Small typo fixes
This is a redo of #1671 (which was bungled) and a realization of #1608.
This changes are all made to the
CONTRIBUTING.mdallowing this file to remain in the repo. This is then copied into the/resources/contributing.mdpage (at the bottom).All the text content here is fair game for edits, so if any language is unsuitable, something is incorrect, or you just don't like something it can be changed or removed.
The idea here was just to give a more detailed outline of how to contribute, so if the workflow for that is wrong, it can be corrected.
I left the translation sections in, but with notices of suspension.
Also looks like I cannot run the
update-external-docs.ymlmyself. This needs to be run to ensure that the script is copied over properly, and for the site contributing page to get populated.