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

docs: tutorial on how to submit a pull request to lb4 #2364

Merged
merged 1 commit into from Feb 26, 2019

Conversation

Projects
None yet
8 participants
@emonddr
Copy link
Contributor

emonddr commented Feb 10, 2019

This tutorial shows contributors step-by-step instructions on how to submit a pull request (PR) to
Loopback v4

@emonddr emonddr requested review from bajtos and raymondfeng as code owners Feb 10, 2019

@emonddr

This comment has been minimized.

Copy link
Contributor Author

emonddr commented Feb 10, 2019

Currently, the intention is to link to this tutorial document from the "Contributing to Loopback"

image

sidebar which is defined in the strongloop/loopback.io repository in the file /loopback.io/_data/sidebars/contrib_sidebar.yml

In the online documentation it is this link : https://loopback.io/doc/en/contrib/doc-contrib.html

If this is agreed upon by everyone, then I will create a PR for that change in the loopback.io repository. :)

@emonddr

This comment has been minimized.

Copy link
Contributor Author

emonddr commented Feb 10, 2019

replaces : strongloop/loopback.io#806 (now closed)

@nabdelgadir
Copy link
Contributor

nabdelgadir left a comment

Looks great 👍, just left some comments/suggestions 😄

Show resolved Hide resolved docs/site/submitting_a_pr.md Outdated
Show resolved Hide resolved docs/site/submitting_a_pr.md Outdated

![submit_pr_create_feature_branch_2.png](./imgs/submit_pr_create_feature_branch_2.png)

Enter the `name` of your new `feature branch` in the search field. Because the branch `emonddr-doc-changes` doesn't yet exist, a menu item to create it appears below the search field. Click on the `Create branch` link.

This comment has been minimized.

@nabdelgadir

nabdelgadir Feb 11, 2019

Contributor

Question: would it be good to also show them how to this with the git command as well?

This comment has been minimized.

@raymondfeng

raymondfeng Feb 11, 2019

Member

It would be much simpler to do so with git command:

git clone <your-fork-of-loopback-next>
cd loopback-next
git checkout -b <branch-name-for-your-pr>

This comment has been minimized.

@raymondfeng

raymondfeng Feb 11, 2019

Member

BTW, we don't have to repeat what's available in github docs - https://help.github.com/categories/collaborating-with-issues-and-pull-requests/. We can simply link to them if necessary.

This comment has been minimized.

@emonddr

emonddr Feb 12, 2019

Author Contributor

@raymondfeng , true...but I was asked in #2280 to give it the "flavour" of this article : https://www.nearform.com/blog/getting-into-open-source-for-the-first-time/ , which does go into some github detail. I figured this is what was desired. Also, is it not more convenient to have the instructions in one place, instead of asking the user to open several other links? I do agree that we can provide a link to that awesome github document you provided in the References section.

This comment has been minimized.

@emonddr

emonddr Feb 13, 2019

Author Contributor

@raymondfeng , in my latest commit, I take the user
through these steps:

git clone <your-fork-of-loopback-next>
cd loopback-next
git checkout -b <branch-name-for-your-pr>

as per your suggestion.

Show resolved Hide resolved docs/site/submitting_a_pr.md Outdated
You can also see this by typing

```
git branch -a

This comment has been minimized.

@nabdelgadir

nabdelgadir Feb 11, 2019

Contributor

Maybe git branch by itself if all they want to see is that they're on the master branch?

This comment has been minimized.

@raymondfeng

raymondfeng Feb 11, 2019

Member

I think we should go directly to git checkout emonddr-doc-changes. The fewer steps, the better. Please bear in the mind that this tutorial is help our contributors to create a good PR as fast as possible. It's not a learning course for git or github.

Show resolved Hide resolved docs/site/submitting_a_pr.md Outdated
Show resolved Hide resolved docs/site/submitting_a_pr.md Outdated

Request a code review or document review.

Add a specific committer to speed up this process. (for example, @bajtos, @raymondfeng).

This comment has been minimized.

@nabdelgadir

nabdelgadir Feb 11, 2019

Contributor

Perhaps you could link to their accounts (e.g. [@bajtos](https://github.com/bajtos)) ?

This comment has been minimized.

@dhmlau

dhmlau Feb 11, 2019

Contributor

tbh, I'm not so sure about adding specific maintainer just to speed up the process. :)
For each package, there is a list of codeowners specified in https://github.com/strongloop/loopback-next/blob/master/CODEOWNERS. They are the maintainers who have deeper knowledge in the specified package and the default reviewers of the PR.
I think we can put it as... if you think there are other contributors should be reviewing your PR, you can add them as one of the reviewers.

This comment has been minimized.

@emonddr

emonddr Feb 12, 2019

Author Contributor

I was just repeating what is being said in
https://loopback.io/doc/en/contrib/code-contrib.html
image

This comment has been minimized.

@emonddr

emonddr Feb 12, 2019

Author Contributor

So should I instead state the a contributor should look into CODEOWNERS file and pick someone from there?

This comment has been minimized.

@nabdelgadir

nabdelgadir Feb 12, 2019

Contributor

Code owners will automatically be requested for review

This comment has been minimized.

@emonddr

emonddr Feb 12, 2019

Author Contributor

oh, very cool.

I changed that paragraph to
"
Once your PR is created, the appropriate reviewer(s) will be notified. This is determined by the configuration settings in /loopback-next/CODEOWNERS.
"

Show resolved Hide resolved docs/site/submitting_a_pr.md Outdated

### 21. Merge your pull request

Finally `merge` your pull request changes into the `master` branch of the `strongloop/loopback-next` [repo](https://github.com/strongloop/loopback-next).

This comment has been minimized.

@nabdelgadir

nabdelgadir Feb 11, 2019

Contributor

Correct me if I'm wrong but I don't think people who aren't maintainers have merging rights.

This comment has been minimized.

@raymondfeng

raymondfeng Feb 11, 2019

Member

We maintainers are responsible for merging the PR.

This comment has been minimized.

@bajtos

bajtos Feb 12, 2019

Member

+1

Let's rephrase this section to make it clear that the contributor has to wait until project maintainers merge the pull request.

Also please consider adding a section explaining what to do after the pull request was merged - how to update master branch in origin to point to the same commit as the master branch in upstream.

This comment has been minimized.

@emonddr

emonddr Feb 13, 2019

Author Contributor

@bajtos , I have addressed
"
Let's rephrase this section to make it clear that the contributor has to wait until project maintainers merge the pull request.
"

@emonddr

This comment has been minimized.

Copy link
Contributor Author

emonddr commented Feb 11, 2019

Related to : #2280


Here is a tutorial on how to submit a pull request (PR) for loopback v4.

### 1. Log in to github

This comment has been minimized.

@raymondfeng

raymondfeng Feb 11, 2019

Member

Before we dive into the details, we should outline the steps for two levels:

  • github newbies
    • (detailed steps)
  • github experts
    • (loopback-next specifics)

This comment has been minimized.

@emonddr

emonddr Feb 13, 2019

Author Contributor

@raymondfeng , I haven't yet created the "expert" list yet. I have mainly addressed all the other comments. But I will address it soon.

Type `q` to quit viewing the list of branches.


### 7. Set up remote tracking

This comment has been minimized.

@raymondfeng

raymondfeng Feb 11, 2019

Member

It is only required if the PR needs to be rebased against the official master. We should move this section to advanced steps.

This comment has been minimized.

@emonddr

emonddr Feb 12, 2019

Author Contributor

@raymondfeng , while I worked on this tutorial with a fake PR request in my forked repo, I had to rebase 2 or 3 times. I agree that it is an advanced topic, but my tutorial doesn't break itself into basic and advanced sections...it lists all detailed steps in one long list. It is a thorough tutorial for beginners/intermediates. I do agree with you ( from another comment) that we should offer 2 choices for a user to follow. 1) Steps for an advanced user (with barely any content..just an outline), and 2) Steps for a beginner/intermediate (with the detailed steps and pictures as I have been providing).


Before `staging your changes` (the next step), be sure your files are free of formatting, syntax, and logical/execution errors.

To flush out any **formatting** issues in code, run `npm run lint`. If there are issues, run `npm run lint:fix`.

This comment has been minimized.

@raymondfeng

raymondfeng Feb 11, 2019

Member

We should recommend npm run lint:fix directly.

This comment has been minimized.

@raymondfeng

raymondfeng Feb 11, 2019

Member

Please note that npm test will report lint errors.

This comment has been minimized.

@emonddr

emonddr Feb 12, 2019

Author Contributor

@nabdelgadir mentioned the same thing. Thanks @raymondfeng .


To flush out any **logical/execution** issues in code, run `npm run test`. This will run the existing mocha test cases, and any new test cases you have added. If there are issues, fix them, run `npm build`, and run `npm run test` again. Repeat until all issues are gone.

### 11. Verifying your documentation changes

This comment has been minimized.

@raymondfeng

raymondfeng Feb 11, 2019

Member

We can probably leave it to CI.

This comment has been minimized.

@emonddr

emonddr Feb 12, 2019

Author Contributor

I liked your earlier statement:
"
Maybe the simplest approach is npm run lint:fix && npm test.
"
Also, doesn't CI simply run the same thing that a user runs on his laptop? If so, it would save them time to run on the laptop first, to flush out all problems locally, before submitting the PR.




### 12. Staging your changes

This comment has been minimized.

@raymondfeng

raymondfeng Feb 11, 2019

Member

We should refer to git/github docs to teach folks how to stage/commit/push.

This comment has been minimized.

@emonddr

emonddr Feb 12, 2019

Author Contributor

I was asked to use https://www.nearform.com/blog/getting-into-open-source-for-the-first-time/ as inspiration for my document, so that's the "flavour" I followed. We can certainly provide a link to github docs in the References. Like the document you mentioned: https://help.github.com/categories/collaborating-with-issues-and-pull-requests/.


Please read the [Commit Message Format](https://loopback.io/doc/en/contrib/code-contrib-lb4.html#commit-message-guidelines) guidelines to correctly format your commit message.

To help with abiding by the rules of commit messages, please use the `commitizen` tool mentioned in the documentation above. This means will we use `git cz` instead of `git commit`.

This comment has been minimized.

@raymondfeng

raymondfeng Feb 11, 2019

Member

git cz is not available unless you run npm install -g commitizen.

This comment has been minimized.

@bajtos
![submit_pr_git_push_2.png](./imgs/submit_pr_git_push_2.png)


### 15. Rebasing

This comment has been minimized.

@raymondfeng

raymondfeng Feb 11, 2019

Member

We should add a section to describe how to add more commits to a given PR. New users are often confused as they don't know the PR automatically picks up the latest version in your branch against the base. There is no need to close the current PR and reopen a new one.

We also should describe what's best practice to add commits to a PR under review.

This comment has been minimized.

@emonddr

emonddr Feb 12, 2019

Author Contributor

I actually have a question about commits "before a PR" and "after a PR".

When someone creates a PR for the first time, is it expected that there is 1 commit with a valid commitizen message? Or does this only matter ONLY before the maintainers merge the PR?

Same question after the PR is created (let's say with 1 commit and valid commitizen message). During the review process, when many commits are added, do they all have to have valid commitizen messages, or does this only matter ONLY before the maintainers merge the PR?

I asked a few weeks ago, and I was told that every commit message should be a valid commitizen message.

This comment has been minimized.

@emonddr

emonddr Feb 13, 2019

Author Contributor

@raymondfeng , I spoke with @b-admike this morning to address my questions above. My latest commit discusses the number of commits before creating a PR, and when it is about to be merged by maintainers.

![submit_pr_squash_commits_10.png](./imgs/submit_pr_squash_commits_10.png)


### 17. Creating the pull request (PR)

This comment has been minimized.

@raymondfeng

raymondfeng Feb 11, 2019

Member

PR can be created before squashing commits, which is usually the final step before the PR is ready to merge.

@dhmlau
Copy link
Contributor

dhmlau left a comment

very detailed instructions! I have a few comments / suggestions. Thanks.

![submit_pr_git_push_2.png](./imgs/submit_pr_git_push_2.png)


### 15. Rebasing

This comment has been minimized.

@dhmlau

dhmlau Feb 11, 2019

Contributor

I think for step 15 and 16, we should create a separate section for that.
"What if your PR gets out of date"?
"What if you have multiple commits and want to squash them into one?"

Not all PRs need to go through those 2 steps but in case contributors need to do it, they can follow the instructions there.


Request a code review or document review.

Add a specific committer to speed up this process. (for example, @bajtos, @raymondfeng).

This comment has been minimized.

@dhmlau

dhmlau Feb 11, 2019

Contributor

tbh, I'm not so sure about adding specific maintainer just to speed up the process. :)
For each package, there is a list of codeowners specified in https://github.com/strongloop/loopback-next/blob/master/CODEOWNERS. They are the maintainers who have deeper knowledge in the specified package and the default reviewers of the PR.
I think we can put it as... if you think there are other contributors should be reviewing your PR, you can add them as one of the reviewers.

Show resolved Hide resolved docs/site/submitting_a_pr.md
Show resolved Hide resolved docs/site/submitting_a_pr.md
lang: en
keywords: contributing, LoopBack community, pull request, PR, loopback
sidebar: contrib_sidebar
permalink: /doc/en/lb4/submitting_a_pr.html

This comment has been minimized.

@bajtos

bajtos Feb 12, 2019

Member

How about /doc/en/lb4/submitting_patches.html?

This comment has been minimized.

@emonddr

emonddr Feb 12, 2019

Author Contributor

@bajtos , the title is "submitting a pull request", the MD file is "submitting_a_pr.md", so I figured that we should keep the HTML file name consistent with "submitting_a_pr.html" . Do we want to change the title of the document?


Please read the [Commit Message Format](https://loopback.io/doc/en/contrib/code-contrib-lb4.html#commit-message-guidelines) guidelines to correctly format your commit message.

To help with abiding by the rules of commit messages, please use the `commitizen` tool mentioned in the documentation above. This means will we use `git cz` instead of `git commit`.

This comment has been minimized.

@bajtos

### 21. Merge your pull request

Finally `merge` your pull request changes into the `master` branch of the `strongloop/loopback-next` [repo](https://github.com/strongloop/loopback-next).

This comment has been minimized.

@bajtos

bajtos Feb 12, 2019

Member

+1

Let's rephrase this section to make it clear that the contributor has to wait until project maintainers merge the pull request.

Also please consider adding a section explaining what to do after the pull request was merged - how to update master branch in origin to point to the same commit as the master branch in upstream.

@emonddr

This comment has been minimized.

Copy link
Contributor Author

emonddr commented Feb 12, 2019

@bajtos , regarding
"
Let's rephrase this section to make it clear that the contributor has to wait until project maintainers merge the pull request.
"
Thanks for the clarification. In my old team, I could merge the items. lol. And my 'deploy to kubernetes on ibm cloud' article, I couldn't remember if I merged it after everyone approved, or if the CICD process merged it after everyone approved. I will change that paragraph.

@emonddr

This comment has been minimized.

Copy link
Contributor Author

emonddr commented Feb 12, 2019

@bajtos ,

concerning:

"
Also please consider adding a section explaining what to do after the pull request was merged - how to update master branch in origin to point to the same commit as the master branch in upstream
"

I was wondering if we should have the user

  1. delete his feature branch
  2. delete his forked repository

Will they be using any of these afterwards...if everything is fine?

This article, mentioned in #2280, https://www.nearform.com/blog/getting-into-open-source-for-the-first-time/, has some clean up steps at the end. Deleting the feature branch of the forked repo, and then rebasing so the master branch of the forked repo is up to date with original repo. But what is the point? Isn't it cleaner and more efficient to delete everything that is no longer needed? I would appreciated your feedback in this matter. thanks. :)

@jannyHou
Copy link
Contributor

jannyHou left a comment

Good stuff. The steps have covered all the scenarios I met when reviewing community PRs.
I left some comments.


To flush out any **syntax** issues in code, run `npm run build`. If there are issues, fix them, and run `npm run build` again. Repeat until all issues are gone.

To flush out any **logical/execution** issues in code, run `npm run test`. This will run the existing mocha test cases, and any new test cases you have added. If there are issues, fix them, run `npm build`, and run `npm run test` again. Repeat until all issues are gone.

This comment has been minimized.

@jannyHou

jannyHou Feb 13, 2019

Contributor

+1 ^

I usually run npm test to make sure all tests pass first then fix the lint error. And npm run lint automatically runs as the post test script.
If tests are all green but npm run lint fails, I run npm run lint:fix or manually fix the formatting issues(some cannot be fixed by script)


To flush out any **syntax** issues in code, run `npm run build`. If there are issues, fix them, and run `npm run build` again. Repeat until all issues are gone.

To flush out any **logical/execution** issues in code, run `npm run test`. This will run the existing mocha test cases, and any new test cases you have added. If there are issues, fix them, run `npm build`, and run `npm run test` again. Repeat until all issues are gone.

This comment has been minimized.

@jannyHou

jannyHou Feb 13, 2019

Contributor

And my understanding for command npm run build is it mainly generates the codes in dist folder.

If you read the scripts in package.json: https://github.com/strongloop/loopback-next/blob/master/package.json#L48-L55

clean and build scripts are run as pretest and lint script is run as posttest, which means npm run test starts from cleaning your workspace, rebuilds your project to make sure the compiled codes are based on the latest src file, runs all tests, finally checks the format.


The next prompt asks if any `breaking changes` will be introduced.

![submit_pr_git_cz_8.png](./imgs/submit_pr_git_cz_8.png)

This comment has been minimized.

@jannyHou

jannyHou Feb 13, 2019

Contributor

I like this prompts that forces people rethink if their code contain breaking change.

Type :

```
git push

This comment has been minimized.

@jannyHou

jannyHou Feb 13, 2019

Contributor

I am not sure if git push works here...usually I run git push origin +{branch_name}. And especially after a rebasing, I have to force push the commits.

This comment has been minimized.

@emonddr

emonddr Feb 13, 2019

Author Contributor

works for me :)

This comment has been minimized.

@emonddr

emonddr Feb 13, 2019

Author Contributor

I see what you mean, @jannyHou . If a user creates the feature branch on the web ui, and then clones from command line, they can do : git push .

But if user forks in the web ui, and then creates the feature branch from the command line, they cannot do a simple : git push .

This comment has been minimized.

@raymondfeng

raymondfeng Feb 25, 2019

Member

git push command will prompt how to track the remote branch.


I want to keep the commit message from the first commit, so I will leave the word `pick` in front of the first commit, and change the words from `pick` to `squash` for the second and third commits. (Use the `arrow` keys to move around, and the `delete` or `backspace` key for deleting characters)

![submit_pr_squash_commits_5.png](./imgs/submit_pr_squash_commits_5.png)

This comment has been minimized.

@jannyHou

jannyHou Feb 13, 2019

Contributor

A nitpick for the commands: you can a single letter for short, like

p 88bc27c5 docs: some description
s 67ac02a7 docs: some other description

This comment has been minimized.

@emonddr

emonddr Feb 13, 2019

Author Contributor

hmm, for beginners/intermediates... I prefer to put full words. :)


Please read the [Commit Message Format](https://loopback.io/doc/en/contrib/code-contrib-lb4.html#commit-message-guidelines) guidelines to correctly format your commit message.

To help with abiding by the rules of commit messages, please use the `commitizen` tool mentioned in the documentation above. This means will we use `git cz` instead of `git commit`.

This comment has been minimized.

@jannyHou

jannyHou Feb 13, 2019

Contributor

Does git cz support add sign-off message? After switching to DCO every commit needs author to sign off.
Would be equivalent to git commit -s -m 'fix: some description'

This comment has been minimized.

@emonddr

emonddr Feb 13, 2019

Author Contributor

commitizen does support signoff. when you enter the long description, you specify:

my long description.\n\nSigned-off-by: Dominique Emond <dremond@ca.ibm.com>

and this would provide a commit message like:

docs: tutorial on how to submit a pull request to lb4
    
This tutorial shows contributors step-by-step instructions on how to submit a pull request (PR) to Loopback v4  

Signed-off-by: Dominique Emond <dremond@ca.ibm.com>

I initially prepared my tutorial for steps involving DCO, but @dhmlau has informed me to prepare it with the CLA approach; since DCO is not approach is not 100% certain.

@emonddr

This comment has been minimized.

Copy link
Contributor Author

emonddr commented Feb 13, 2019

@raymondfeng @bajtos @dhmlau , when someone first creates the pull request, is it important that rebasing and squashing of commits has been done? Don't the maintainers want 1 commit to look at, and 1 clear commit message?

Then, during the review phase, the contributor adds more commits with messages.

Then, at the end, there is a final rebasing and squashing of commits ?

The reason I am asking is because I want to know if the Rebasing section and Squashing Commits section should go after the Go through the PR review process.

I know that if I were a maintainer, I would like to look at 1 commit, and 1 commit message...at least initially.

This article, https://www.nearform.com/blog/getting-into-open-source-for-the-first-time/, mentioned in #2280, also mentions rebasing just before creating a pull request.

He doesn't mention squashing commits, however.

Today I spoke with Biniam , @b-admike , about this and he clarified a few things. My latest commit has my changes.

@dhmlau

This comment has been minimized.

Copy link
Contributor

dhmlau commented Feb 13, 2019

when someone first creates the pull request, is it important that rebasing and squashing of commits has been done?

There are different point in time that the user needs to rebase and squash the commits. Here are my thoughts:

  • if your PR is ready for review, it's important that your PR is up-to-date (so rebased if you find the PR is outdated). However, the master branch has updates fairly frequently, so it might be hard to keep rebasing while you're just waiting for someone to review (it might take days).
  • during the review process, someone might left some review comments and you've made some changes to the PR, I think it makes sense not to squash the commits right away especially for the bigger PRs, so that it's easier for people to review the delta.
@bschrammIBM

This comment has been minimized.

Copy link
Contributor

bschrammIBM commented Feb 13, 2019

Suggest that you link directly to Contributing code/LoopBack 4 (https://loopback.io/doc/en/contrib/code-contrib-lb4.html) in Step 9. The Contributing code link can open the LB4 page. Currently it links to (http://localhost:4001/doc/en/contrib/code-contrib.html, a 3.x page.

Step 9: Do some work in your local feature branch directory
Whether you are contributing to code or documentation, be sure to make all your changes inside in the local feature branch directory.

Be sure to read, understand, and abide by the instructions provided in Contributing code (http://localhost:4001/doc/en/contrib/code-contrib.html), Contributing to docs, and Contributing to LoopBack
@raymondfeng

This comment has been minimized.

Copy link
Member

raymondfeng commented Feb 14, 2019

I like the detailed walkthrough for github newbies. But 18 steps is scary :-). Please separate the ones that are common to all github repos from those extras/specifics for loopback-next. At the top of the tutorial, allow fluent github users to go directly to our special requirements.

Let's think about how many mins are needed to read and how many steps to go through. It’s critical to keep them minimal. Use the TL;DR style if necessary.

By our experience, most users struggle with or do not pay attention to the following:

  1. Sign CLA
  2. Run npm run lint:fix before commit to fix style errors
  3. Follow conventional commit log and amend wrong ones
  4. Adding more commits to the same PR
  5. Rebase against latest master
  6. Squash commits (edited)

These are the pain points that deserve more words/diagrams.

Use the template to fill in as much information as possible to properly describe
the purpose of the pull request.

{% include tip.html content="The title of the pull request should start with <b>[WIP]</b> for <b>work in progress</b> when the pull request is not complete yet." %}

This comment has been minimized.

@nabdelgadir

nabdelgadir Feb 15, 2019

Contributor

@dhmlau mentioned GitHub now supports draft PRs that we can use instead of [WIP].

This comment has been minimized.

@emonddr

emonddr Feb 25, 2019

Author Contributor

@dmlau , I read about them just now, and there isn't any indication they can be distinguished from normal PRs in the UI. So having [WIP] would still be useful in the title.

@dhmlau
Copy link
Contributor

dhmlau left a comment

Thanks for trimming down the instructions.
In general, I'm thinking perhaps we could further trim down some of the explanation (see review comments) too.

Open a terminal window, and navigate to the directory where you want to clone
the repository. In my case, this is `/Users/dremond/git` .

To create the `feature` branch, run:

This comment has been minimized.

@dhmlau

dhmlau Feb 15, 2019

Contributor

I'm thinking we can even further trim down the above content in this step and starts from here.

This comment has been minimized.

@b-admike

b-admike Feb 21, 2019

Member

Yeah I think if we say follow the steps on how to clone the repository and link them to https://help.github.com/en/articles/cloning-a-repository, it would be sufficient.

It is not necessary to create a pull request immediately on the push of your
first commit; this can be done later.

Do take some time to think about how many commits you want in the pull request

This comment has been minimized.

@dhmlau

dhmlau Feb 15, 2019

Contributor

perhaps line 198-204 can be in one paragraph?
I'm thinking whether we need to be verbose about that, or simply show how. Thoughts, @raymondfeng?


### 6. Rebasing

Eventually your fork of the original repository will become stale, and it will

This comment has been minimized.

@dhmlau

dhmlau Feb 15, 2019

Contributor

maybe use if instead of eventually? because someone will be "lucky" enough that the PR got merged before it becomes stale. :)

This comment has been minimized.

@dhmlau

dhmlau Feb 15, 2019

Contributor

Suggested changes:
If your fork becomes stale at any point in time, you can bring it up-to-date by rebasing your PR.

<here are the steps>

then it goes directly to ur next snippet git remote...

This comment has been minimized.

@raymondfeng

raymondfeng Feb 25, 2019

Member

+1. stale is a bit confusing too. Maybe we should say your branch is behind the original/master.

git push --force-with-lease origin { your feature branch }
```

Now we can see that the copy of the repository is no longer behind in several

This comment has been minimized.

@dhmlau

dhmlau Feb 15, 2019

Contributor

maybe we don't need line 241-251?

This comment has been minimized.

@raymondfeng

raymondfeng Feb 25, 2019

Member

We can directly rebase against origin/master for your feature branch:

git checkout { your feature branch }
git rebase upstream/master (or git pull --rebase upstream master)
git push --force-with-lease
It a **necessary** step `after` the pull request has been created,
reviewed, approved and is about to be merged by the maintainers.

Before creating a pull request, you can choose to squash your commits into one

This comment has been minimized.

@dhmlau

dhmlau Feb 15, 2019

Contributor

similar for this paragraph and next... I'm thinking whether we can just skip this and just show the how.


![submit_pr_rebase_2.png](./imgs/submit_pr_rebase_2.png)

### 7. Squashing commits

This comment has been minimized.

@raymondfeng

raymondfeng Feb 15, 2019

Member

We usually squash commit after the code review and before merge.

@dhmlau

dhmlau approved these changes Feb 19, 2019

Copy link
Contributor

dhmlau left a comment

I have 3 new review comments on some nitpicks (GitHub and the extra space after bracket), other than that, LGTM.
Please get a review from @raymondfeng to make sure his idea of splitting the new vs experienced GH users is captured.


Here are instructions on how to submit a pull request (PR) for LoopBack v4.

If you are a github expert, please follow the

This comment has been minimized.

@dhmlau

dhmlau Feb 19, 2019

Contributor

github -> GitHub

If you are a github expert, please follow the
[Expert instructions](#expert-instructions) below.

If you are a github beginner, please follow the

This comment has been minimized.

@dhmlau

dhmlau Feb 19, 2019

Contributor

github -> GitHub


#### 2. Create the feature branch

Notice your repo has a `master` branch already created ( refer bottom left

This comment has been minimized.

@dhmlau

dhmlau Feb 19, 2019

Contributor

extra space after (

npm run lint:fix && npm run test
```

To ensure your `documentation` files are free of formatting errors, run :

This comment has been minimized.

@raymondfeng

raymondfeng Feb 19, 2019

Member

I think we can defer this step to CI.

click on `LoopBack 4`, and verify that your documentation changes appear and are
well-rendered.

#### 4. Conventional Commits

This comment has been minimized.

@raymondfeng

raymondfeng Feb 19, 2019

Member

IMO, this section should be the 1st one as we want to make sure our contributors follow it during commits before a PR is created.

I prefer to order these steps based on contributors' flow. For example:

  • during commit to local repo (Conventional Commits)
  • before push to remote repo (npm run lint:fix && npm test)
  • right after create a PR (sign CLA)
  • after CI reports status for the PR (Check CI failures)
  • before merge (Clean up commit history if needed)
### Expert Instructions

In addition to your knowledge about github and creating a pull request, we have
additional steps you need to follow when submitting a pull request for

This comment has been minimized.

@raymondfeng

raymondfeng Feb 19, 2019

Member

additional steps --> specific conventions


Our commit messages are formatted according to
[Conventional Commits](https://conventionalcommits.org/).

This comment has been minimized.

@raymondfeng

raymondfeng Feb 19, 2019

Member

We should advise contributors to run npm i for loopback-next. It will create a commit-msg hook under loopback-next/.git/pre-commit, which enforces the conventional commit rules automatically.

@b-admike
Copy link
Member

b-admike left a comment

Well done @emonddr 👍 pretty good read and guide with explicit examples along the way. I like the way it's split for advanced users and new ones as @raymondfeng and others have suggested. There might be more areas we can trim down as @dhmlau pointed out.

This tutorial is about how to submit a pull request (PR) for LoopBack 4 while
following our conventions and requirements.

Pick `Expert Instructions` or `Beginner Instructions` mode based your experience

This comment has been minimized.

@b-admike

b-admike Feb 21, 2019

Member

nitpick: based your -> based on your

#### 2. Before pushing to remote repository

Before pushing to the remote repository, ensure your files are free of
formatting, syntax, and logical/execution errors, by running :

This comment has been minimized.

@b-admike

b-admike Feb 21, 2019

Member

nitpick: running : -> running: (I personally leave the comma out before by running too, but feel free to keep it)

The review process is iterative, so it is normal that `multiple commits` may be
required until the pull request is finally accepted.

{% include tip.html content="After a pull request is created, any additional commits that you push to your remote feature branch automatically get added to the pull request. There is no need to close the initial pull request and create a new one." %}

This comment has been minimized.

@b-admike
{% include tip.html content="After a pull request is created, any additional commits that you push to your remote feature branch automatically get added to the pull request. There is no need to close the initial pull request and create a new one." %}

Reactively [rebase](https://help.github.com/articles/about-git-rebase/) your
forked repository against the upstream master branch to keep them in synch; if

This comment has been minimized.

@b-admike

b-admike Feb 21, 2019

Member

nitpick: synch -> sync perhaps?

forked repository against the upstream master branch to keep them in synch; if
needed.

#### Before your PR is merged by maintainer

This comment has been minimized.

@b-admike

b-admike Feb 21, 2019

Member

nitpick: by maintainer -> by a maintainer or by maintainers

This brings up tiny dialog with different choices : Clone with SSH, Use HTTPS,
Open in Desktop, or Download Zip.

In my case, I will leave it as `Clone with SSH`, and click on the

This comment has been minimized.

@b-admike

b-admike Feb 21, 2019

Member

Can we leave a note here that Clone with SSH option requires you to have SSH set up on your local client machine? If they use HTTPS, I believe they would have to enter their GitHub credentials.

This comment has been minimized.

@emonddr

emonddr Feb 21, 2019

Author Contributor

hmm. I keep getting told that I have to trim information down. Not sure I want to start showing them the steps of using https. they can look it up.

Open a terminal window, and navigate to the directory where you want to clone
the repository. In my case, this is `/Users/dremond/git` .

To create the `feature` branch, run:

This comment has been minimized.

@b-admike

b-admike Feb 21, 2019

Member

Yeah I think if we say follow the steps on how to clone the repository and link them to https://help.github.com/en/articles/cloning-a-repository, it would be sufficient.

inside in the local feature branch directory.

To ensure your files are free of formatting, syntax, and logical/execution
errors, run :

This comment has been minimized.

@b-admike

b-admike Feb 21, 2019

Member

nitpick: run : -> run:

```

To start `commitizen`, run :

This comment has been minimized.

@b-admike

b-admike Feb 21, 2019

Member

ditto with run : -> run: and anywhere else you might have that space (don't want to bombard you with nitpick comments :-) )

```
git remote add upstream git@github.com:strongloop/loopback-next.git
git checkout master
git pull --rebase upstream master

This comment has been minimized.

@b-admike

b-admike Feb 21, 2019

Member

Does this rebase command go into an interactive rebase?

This comment has been minimized.

@emonddr

emonddr Feb 21, 2019

Author Contributor

not it just pulls everything down.

formatting, syntax, and logical/execution errors, by running :

```
npm run lint:fix && npm run test

This comment has been minimized.

@raymondfeng

raymondfeng Feb 21, 2019

Member

npm run lint:fix && npm test

Please note that npm run lint:fix might reformat the source code and fix style issues. Please add such changes to your commit.

This comment has been minimized.

@raymondfeng

raymondfeng Feb 21, 2019

Member

Please change to npm run lint:fix && npm test


#### 3. After creating PR, sign the CLA and fill out checklist

After creating the pull request, sign the Contributor License Agreement (CLA).

This comment has been minimized.

@raymondfeng

raymondfeng Feb 21, 2019

Member

make sure the Contributor License Agreement (CLA) has been signed for the loopback-next repository.

This tutorial is about how to submit a pull request (PR) for LoopBack 4 while
following our conventions and requirements.

Pick `Expert Instructions` or `Beginner Instructions` mode based your experience

This comment has been minimized.

@raymondfeng

raymondfeng Feb 21, 2019

Member

Please add in-page links to each mode.

#### 4. Check CI status

Ensure that the continuous integration (CI) jobs associated with your pull
request complete successfully.

This comment has been minimized.

@raymondfeng

raymondfeng Feb 21, 2019

Member

It might take some time for CI jobs to be scheduled and completed.

This comment has been minimized.

@raymondfeng

raymondfeng Feb 21, 2019

Member

We should tell contributors to click on Details for failing items to check. It might be nice to have a screenshot of travis CI report.

@emonddr

This comment has been minimized.

Copy link
Contributor Author

emonddr commented Feb 21, 2019

@b-admike , concerning
"
Yeah I think if we say follow the steps on how to clone the repository and link them to https://help.github.com/en/articles/cloning-a-repository, it would be sufficient.
"
I am also showing them how to create their own feature branch after cloning, so I don't want to break their focus. Personally, I don't like tutorials that point to many outside links for steps, instead of showing me inline. References section..sure..we can have many related links etc.

@emonddr

This comment has been minimized.

Copy link
Contributor Author

emonddr commented Feb 21, 2019

@nabdelgadir suggested we can place one link to my document here:

image

and a link to my document somewhere here:

image

since a PR can be for "code" or for "docs"


Run `npm install` inside `loopback-next` after `git clone`. This will
automatically set up git `commit-msg` hooks to check the conventions and block
invalid messages.

This comment has been minimized.

@raymondfeng

raymondfeng Feb 21, 2019

Member

IIRC, there is a macro we can use to highlight the paragraph as NOTE.

This comment has been minimized.

@raymondfeng

raymondfeng Feb 23, 2019

Member
{% include note.html content=...%}

This comment has been minimized.

@emonddr

emonddr Feb 25, 2019

Author Contributor

@raymondfeng addressed this comment.

![submit_pr_create_feature_branch_2.png](./imgs/submit_pr_create_feature_branch_2.png)

Open a terminal window, and navigate to the directory where you want to clone
the repository. In my case, this is `/Users/dremond/git` .

This comment has been minimized.

@raymondfeng

raymondfeng Feb 25, 2019

Member

nitpick: Using git as the directory is confusing. It would be better to use something like /Users/dremond/Projects.

To stage one untracked file at a time, run:

```
git add { relative path to file from root directory }

This comment has been minimized.

@raymondfeng

raymondfeng Feb 25, 2019

Member

We can add a tip to use git gui.


If, however, it makes more sense to place your changes into a few logically
separate commits, then do so.

This comment has been minimized.

@raymondfeng

raymondfeng Feb 25, 2019

Member

This paragraph does not add much value unless we have some best practice for them to follow.

@dhmlau

dhmlau approved these changes Feb 25, 2019

forked repository against the upstream master branch to keep them in sync; if
needed.

#### Before your PR is merged by a maintainer

This comment has been minimized.

@raymondfeng

raymondfeng Feb 25, 2019

Member

Missing the item number such as #### 6. ...

This comment has been minimized.

@emonddr

emonddr Feb 25, 2019

Author Contributor

good catch. How did I miss that? oh well.

@raymondfeng
Copy link
Member

raymondfeng left a comment

Please fix https://github.com/strongloop/loopback-next/pull/2364/files#r260033805 and squash commits before merge.

@emonddr

This comment has been minimized.

Copy link
Contributor Author

emonddr commented Feb 25, 2019

@raymondfeng , addressed latest comment, rebased, and squashed commits into one.

docs: tutorial on how to submit a pull request to lb4
This tutorial shows contributors step-by-step instructions on how to submit a pull request (PR) to
Loopback v4

@emonddr emonddr merged commit af4a55c into strongloop:master Feb 26, 2019

4 checks passed

clahub All contributors have signed the Contributor License Agreement.
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage remained the same at 90.637%
Details

@emonddr emonddr deleted the emonddr:emonddr_submit_pr_doc branch Feb 26, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment