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.

Sign up for GitHub

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

Jump to bottom

doc: make pull-request guide default branch agnostic #41299

Merged
merged 3 commits into from Dec 31, 2021
Merged

doc: make pull-request guide default branch agnostic #41299

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
a674c6f
doc: make pull-request guide default branch agnostic
aduh95 Dec 23, 2021
adb7ed3
Use `text` instead of `sh`
aduh95 Dec 26, 2021
bace3a8
Update doc/guides/contributing/pull-requests.md
aduh95 Dec 26, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
76 changes: 34 additions & 42 deletions doc/guides/contributing/pull-requests.md
View file
Open in desktop
Expand Up @@ -71,18 +71,18 @@ it's time to create a fork.
Fork the project [on GitHub](https://github.com/nodejs/node) and clone your fork
locally.

```text
$ git clone git@github.com:username/node.git
$ cd node
$ git remote add upstream https://github.com/nodejs/node.git
$ git fetch upstream
```sh
Copy link
Member

Choose a reason for hiding this comment

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

Non-blocking but this change results in wonky highlighting that makes no sense to me.

image

This is because sh makes it think it's syntax-highlighting a bash script rather than rendering shell CLI stuff. I think txt is better.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think text make no sense semantically speaking. I don't feel strongly about syntax highlighting though, maybe we should disable syntax highlighting for (ba)sh altogether? From a quick glance it looks like it's never useful (and sometimes it even adds confusion as this one). If that sounds good to you, I can open a PR doing that.

Copy link
Member

Choose a reason for hiding this comment

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

If that sounds good to you, I can open a PR doing that.

Is it possible to do that? This is a document that gets displayed in the GitHub interface, not on our web pages.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah no, we don't have control over this indeed, I thought you meant the HTML version of the docs

Copy link
Member

Choose a reason for hiding this comment

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

A valid flag in Linguist (which is what the GitHub interface uses) is fundamental. It's an alias for text but maybe it is more semantically tolerable?

Another possibility is to adjust our rules to omit use of a flag when it makes sense.

And one more possibility would be to not use code blocks for this sort of thing. Like, in HTML, these kind of code blocks might be a <code> elements but maybe these kind of "here's a list of commands to run" should really be <pre> elements or something else?

Copy link
Member

@Trott Trott Dec 25, 2021 •
edited

Choose a reason for hiding this comment

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

And of course the two status quo options:

  1. Don't worry about the wonky highlighting.
  2. Use text and either convince ourselves that it makes sense semantically or don't worry that maybe it doesn't.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If I had unlimited time and motivation, I would track down which library GitHub is using for syntax highlighting and beg them to get rid of this keyword highlighting which is more confusing than helping. But since I don’t: I think, like the bash vs sh discussion we had, since the semantic is not really a user facing feature, it should not matter and we should use whatever works with the current config (so text). I would still be very interested to know if the semantic part resonates with others, in particular Id be interested to know if that makes any difference to folks using a screen reader to collaborate.

Copy link
Member

@Trott Trott Dec 25, 2021 •
edited

Choose a reason for hiding this comment

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

If I had unlimited time and motivation, I would track down which library GitHub is using for syntax highlighting and beg them to get rid of this keyword highlighting which is more confusing than helping.

GitHub uses https://github.com/github/linguist.

Valid flags are definied in https://github.com/github/linguist/blob/master/lib/linguist/languages.yml.

They say they use Linguist for detection but it's not clear to me that it's used for actual highlighting.

I would still be very interested to know if the semantic part resonates with others, in particular Id be interested to know if that makes any difference to folks using a screen reader to collaborate.

Since screen readers use the HTML output in the GitHub interface, and the language flags end up as CSS, class attributes, and things like that, I doubt they play a semantic role for many from an a11y perspective. (But the weird highlighting might.) The exception would be someone reading raw markdown (whether with assistive technology or not).

git clone git@github.com:username/node.git
cd node
git remote add upstream https://github.com/nodejs/node.git
git fetch upstream
```

Configure `git` so that it knows who you are:

```text
$ git config user.name "J. Random User"
$ git config user.email "j.random.user@example.com"
```sh
git config user.name "J. Random User"
git config user.email "j.random.user@example.com"
```

You can use any name/email address you prefer here. We only use the
Expand All @@ -98,10 +98,10 @@ make sure this local email is also added to your

As a best practice to keep your development environment as organized as
possible, create local branches to work within. These should also be created
directly off of the `master` branch.
directly off of the upstream default branch.

```text
$ git checkout -b my-branch -t upstream/master
$ git checkout -b my-branch -t upstream/HEAD
```

## The process of making changes
Expand Down Expand Up @@ -218,13 +218,12 @@ As a best practice, once you have committed your changes, it is a good idea
to use `git rebase` (not `git merge`) to synchronize your work with the main
repository.

```text
$ git fetch upstream
$ git rebase upstream/master
```sh
git fetch upstream HEAD
git rebase FETCH_HEAD
```

This ensures that your working branch has the latest changes from `nodejs/node`
master.
This ensures that your working branch has the latest changes from `nodejs/node`.

### Step 6: Test

Expand Down Expand Up @@ -261,8 +260,8 @@ Once you are sure your commits are ready to go, with passing tests and linting,
begin the process of opening a pull request by pushing your working branch to
your fork on GitHub.

```text
$ git push origin my-branch
```sh
git push origin my-branch
```

### Step 8: Opening the pull request
Expand Down Expand Up @@ -290,33 +289,26 @@ To make changes to an existing pull request, make the changes to your local
branch, add a new commit with those changes, and push those to your fork.
GitHub will automatically update the pull request.

```text
$ git add my/changed/files
$ git commit
$ git push origin my-branch
```sh
git add my/changed/files
git commit
git push origin my-branch
```

It is also frequently necessary to synchronize your pull request with other
changes that have landed in `master` by using `git rebase`:
If a git conflict arises, it is necessary to synchronize your branch with other
changes that have landed upstream by using `git rebase`:

```text
$ git fetch --all
$ git rebase upstream/master
$ git push --force-with-lease origin my-branch
```sh
git fetch upstream HEAD
git rebase FETCH_HEAD
git push --force-with-lease origin my-branch
```

**Important:** The `git push --force-with-lease` command is one of the few ways
to delete history in `git`. Before you use it, make sure you understand the
risks. If in doubt, you can always ask for guidance in the pull request.

If you happen to make a mistake in any of your commits, do not worry. You can
amend the last commit (for example if you want to change the commit log).

```text
$ git add any/changed/files
$ git commit --amend
$ git push --force-with-lease origin my-branch
```
to delete history in `git`. It also complicates the review process, as it won't
allow reviewers to get a quick glance on what changed. Before you use it, make
sure you understand the risks. If in doubt, you can always ask for guidance in
the pull request.

There are a number of more advanced mechanisms for managing commits using
`git rebase` that can be used, but are beyond the scope of this guide.
Expand Down Expand Up @@ -356,10 +348,10 @@ your pull request waiting longer than you expect, see the

When a collaborator lands your pull request, they will post
a comment to the pull request page mentioning the commit(s) it
landed as. GitHub often shows the pull request as `Closed` at this
landed as. GitHub may show the pull request as `Closed` at this
aduh95 marked this conversation as resolved.
Show resolved Hide resolved
point, but don't worry. If you look at the branch you raised your
pull request against (probably `master`), you should see a commit with
your name on it. Congratulations and thanks for your contribution!
pull request against, you should see a commit with your name on it.
Congratulations and thanks for your contribution!

## Reviewing pull requests

Expand Down Expand Up @@ -542,7 +534,7 @@ For the size of "one logical change",
[0b5191f](https://github.com/nodejs/node/commit/0b5191f15d0f311c804d542b67e2e922d98834f8)
can be a good example. It touches the implementation, the documentation,
and the tests, but is still one logical change. All tests should always pass
when each individual commit lands on the master branch.
when each individual commit lands on one of the `nodejs/node` branches.

### Getting approvals for your pull request

Expand Down