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

emonddr · 2019-02-10T22:20:16Z

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

emonddr · 2019-02-10T23:09:27Z

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

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 · 2019-02-10T23:20:00Z

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

nabdelgadir

Looks great 👍, just left some comments/suggestions 😄

docs/site/submitting_a_pr.md

nabdelgadir · 2019-02-11T01:59:38Z

docs/site/submitting_a_pr.md

+
+![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.


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

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>

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.

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

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

docs/site/submitting_a_pr.md

nabdelgadir · 2019-02-11T02:03:18Z

docs/site/submitting_a_pr.md

+You can also see this by typing
+
+```
+git branch -a


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

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.

docs/site/submitting_a_pr.md

nabdelgadir · 2019-02-11T02:25:50Z

docs/site/submitting_a_pr.md

+
+Request a code review or document review.
+
+Add a specific committer to speed up this process. (for example, @bajtos, @raymondfeng).


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

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.

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

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

Code owners will automatically be requested for review

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

docs/site/submitting_a_pr.md

nabdelgadir · 2019-02-11T02:27:43Z

docs/site/submitting_a_pr.md

+
+### 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).


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

We maintainers are responsible for merging the PR.

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

@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 · 2019-02-11T13:57:17Z

Related to : #2280

raymondfeng · 2019-02-11T18:36:30Z

docs/site/submitting_a_pr.md

+
+Here is a tutorial on how to submit a pull request (PR) for loopback v4.
+
+### 1. Log in to github


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

github newbies

(detailed steps)

github experts

(loopback-next specifics)

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

raymondfeng · 2019-02-11T18:44:19Z

docs/site/submitting_a_pr.md

+Type `q` to quit viewing the list of branches.
+
+
+### 7. Set up remote tracking


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

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

raymondfeng · 2019-02-11T18:45:07Z

docs/site/submitting_a_pr.md

+
+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`.


We should recommend npm run lint:fix directly.

Please note that npm test will report lint errors.

@nabdelgadir mentioned the same thing. Thanks @raymondfeng .

raymondfeng · 2019-02-11T18:47:21Z

docs/site/submitting_a_pr.md

+
+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


We can probably leave it to CI.

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.

raymondfeng · 2019-02-11T18:48:12Z

docs/site/submitting_a_pr.md

+
+
+
+### 12. Staging your changes


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

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

raymondfeng · 2019-02-11T18:48:55Z

docs/site/submitting_a_pr.md

+
+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`.


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

raymondfeng · 2019-02-11T18:51:43Z

docs/site/submitting_a_pr.md

+![submit_pr_git_push_2.png](./imgs/submit_pr_git_push_2.png)
+
+
+### 15. Rebasing


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.

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.

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

raymondfeng · 2019-02-11T18:53:04Z

docs/site/submitting_a_pr.md

+![submit_pr_squash_commits_10.png](./imgs/submit_pr_squash_commits_10.png)
+
+
+### 17. Creating the pull request (PR)


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

dhmlau

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

dhmlau · 2019-02-11T20:02:15Z

docs/site/submitting_a_pr.md

+![submit_pr_git_push_2.png](./imgs/submit_pr_git_push_2.png)
+
+
+### 15. Rebasing


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.

dhmlau · 2019-02-11T20:05:05Z

docs/site/submitting_a_pr.md

+
+Request a code review or document review.
+
+Add a specific committer to speed up this process. (for example, @bajtos, @raymondfeng).


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.

docs/site/submitting_a_pr.md

bajtos · 2019-02-12T12:28:39Z

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


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

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

bajtos · 2019-02-12T12:33:17Z

docs/site/submitting_a_pr.md

+
+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`.


bajtos · 2019-02-12T12:34:55Z

docs/site/submitting_a_pr.md

+
+### 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).


+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 · 2019-02-12T18:35:39Z

@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 · 2019-02-12T22:56:00Z

@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

delete his feature branch
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

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

jannyHou · 2019-02-13T01:07:51Z

docs/site/submitting_a_pr.md

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


+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)

jannyHou · 2019-02-13T01:17:37Z

docs/site/submitting_a_pr.md

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


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.

jannyHou · 2019-02-13T01:23:03Z

docs/site/submitting_a_pr.md

+
+The next prompt asks if any `breaking changes` will be introduced.
+
+![submit_pr_git_cz_8.png](./imgs/submit_pr_git_cz_8.png)


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

jannyHou · 2019-02-13T01:24:54Z

docs/site/submitting_a_pr.md

+Type :
+
+```
+git push


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.

works for me :)

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 .

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

jannyHou · 2019-02-13T01:28:29Z

docs/site/submitting_a_pr.md

+
+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)


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

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

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

jannyHou · 2019-02-13T01:32:08Z

docs/site/submitting_a_pr.md

+
+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`.


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'

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

my long description.\n\nSigned-off-by: Dominique Emond <[email protected]>

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 <[email protected]>

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 · 2019-02-13T02:36:36Z

@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 · 2019-02-13T15:27:46Z

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 · 2019-02-13T20:39:18Z

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 · 2019-02-14T16:16:24Z

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:

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

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

nabdelgadir · 2019-02-15T17:46:17Z

docs/site/submitting_a_pr.md

+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." %}


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

@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

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.

dhmlau · 2019-02-15T17:36:55Z

docs/site/submitting_a_pr.md

+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:


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

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.

dhmlau · 2019-02-15T17:42:10Z

docs/site/submitting_a_pr.md

+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


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?

dhmlau · 2019-02-15T17:43:10Z

docs/site/submitting_a_pr.md

+
+### 6. Rebasing
+
+Eventually your fork of the original repository will become stale, and it will


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

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

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

dhmlau · 2019-02-15T17:47:57Z

docs/site/submitting_a_pr.md

+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


maybe we don't need line 241-251?

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

dhmlau · 2019-02-15T17:56:09Z

docs/site/submitting_a_pr.md

+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


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

raymondfeng · 2019-02-15T18:10:41Z

docs/site/submitting_a_pr.md

+
+![submit_pr_rebase_2.png](./imgs/submit_pr_rebase_2.png)
+
+### 7. Squashing commits


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

dhmlau

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.

dhmlau · 2019-02-19T15:19:27Z

docs/site/submitting_a_pr.md

+
+Here are instructions on how to submit a pull request (PR) for LoopBack v4.
+
+If you are a github expert, please follow the


github -> GitHub

dhmlau · 2019-02-19T15:19:33Z

docs/site/submitting_a_pr.md

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


github -> GitHub

dhmlau · 2019-02-19T15:21:08Z

docs/site/submitting_a_pr.md

+
+#### 2. Create the feature branch
+
+Notice your repo has a `master` branch already created ( refer bottom left


extra space after (

raymondfeng · 2019-02-19T16:26:09Z

docs/site/submitting_a_pr.md

+npm run lint:fix && npm run test
+```
+
+To ensure your `documentation` files are free of formatting errors, run :


I think we can defer this step to CI.

raymondfeng · 2019-02-19T16:31:15Z

docs/site/submitting_a_pr.md

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


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)

raymondfeng · 2019-02-19T17:42:56Z

docs/site/submitting_a_pr.md

+### 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


additional steps --> specific conventions

raymondfeng · 2019-02-19T17:49:02Z

docs/site/submitting_a_pr.md

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


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

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.

b-admike · 2019-02-21T03:27:10Z

docs/site/submitting_a_pr.md

+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


nitpick: based your -> based on your

b-admike · 2019-02-21T03:32:43Z

docs/site/submitting_a_pr.md

+#### 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 :


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

b-admike · 2019-02-21T03:33:54Z

docs/site/submitting_a_pr.md

+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." %}


b-admike · 2019-02-21T03:34:39Z

docs/site/submitting_a_pr.md

+{% 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


nitpick: synch -> sync perhaps?

b-admike · 2019-02-21T03:35:38Z

docs/site/submitting_a_pr.md

+forked repository against the upstream master branch to keep them in synch; if
+needed.
+
+#### Before your PR is merged by maintainer


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

b-admike · 2019-02-21T03:41:18Z

docs/site/submitting_a_pr.md

+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


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.

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.

b-admike · 2019-02-21T03:42:48Z

docs/site/submitting_a_pr.md

+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:


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.

b-admike · 2019-02-21T03:43:49Z

docs/site/submitting_a_pr.md

+inside in the local feature branch directory.
+
+To ensure your files are free of formatting, syntax, and logical/execution
+errors, run :


nitpick: run : -> run:

b-admike · 2019-02-21T03:45:28Z

docs/site/submitting_a_pr.md

+
+```
+
+To start `commitizen`, run :


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

b-admike · 2019-02-21T03:49:21Z

docs/site/submitting_a_pr.md

+```
+git remote add upstream [email protected]:strongloop/loopback-next.git
+git checkout master
+git pull --rebase upstream master


Does this rebase command go into an interactive rebase?

not it just pulls everything down.

docs/site/submitting_a_pr.md

raymondfeng · 2019-02-21T04:26:36Z

docs/site/submitting_a_pr.md

+formatting, syntax, and logical/execution errors, by running :
+
+```
+npm run lint:fix && npm run test


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.

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

raymondfeng · 2019-02-21T04:28:01Z

docs/site/submitting_a_pr.md

+
+#### 3. After creating PR, sign the CLA and fill out checklist
+
+After creating the pull request, sign the Contributor License Agreement (CLA).


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

raymondfeng · 2019-02-21T04:33:27Z

docs/site/submitting_a_pr.md

+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


Please add in-page links to each mode.

raymondfeng · 2019-02-21T04:36:38Z

docs/site/submitting_a_pr.md

+#### 4. Check CI status
+
+Ensure that the continuous integration (CI) jobs associated with your pull
+request complete successfully.


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

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 · 2019-02-21T15:29:52Z

@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 · 2019-02-21T21:18:23Z

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

and a link to my document somewhere here:

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

raymondfeng · 2019-02-21T21:30:26Z

docs/site/submitting_a_pr.md

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


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

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

@raymondfeng addressed this comment.

raymondfeng · 2019-02-25T15:48:24Z

docs/site/submitting_a_pr.md

+![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` .


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

raymondfeng · 2019-02-25T15:49:28Z

docs/site/submitting_a_pr.md

+To stage one untracked file at a time, run:
+
+```
+git add  { relative path to file from root directory }


We can add a tip to use git gui.

raymondfeng · 2019-02-25T15:52:11Z

docs/site/submitting_a_pr.md

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


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

raymondfeng · 2019-02-25T21:49:53Z

docs/site/submitting_a_pr.md

+forked repository against the upstream master branch to keep them in sync; if
+needed.
+
+#### Before your PR is merged by a maintainer


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

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

raymondfeng

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

emonddr · 2019-02-25T23:06:08Z

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

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

emonddr requested review from bajtos and raymondfeng as code owners February 10, 2019 22:20

emonddr mentioned this pull request Feb 10, 2019

docs: tutorial on how to submit a pull request to lb4 loopbackio/loopback.io#806

Closed

nabdelgadir reviewed Feb 11, 2019

View reviewed changes

raymondfeng reviewed Feb 11, 2019

View reviewed changes

dhmlau reviewed Feb 11, 2019

View reviewed changes

bajtos reviewed Feb 12, 2019

View reviewed changes

bajtos added feature Docs Contributing Things making it easier and more pleasant to contribute to LoopBack labels Feb 12, 2019

jannyHou reviewed Feb 13, 2019

View reviewed changes

nabdelgadir reviewed Feb 15, 2019

View reviewed changes

dhmlau reviewed Feb 15, 2019

View reviewed changes

raymondfeng reviewed Feb 15, 2019

View reviewed changes

dhmlau approved these changes Feb 19, 2019

View reviewed changes

raymondfeng reviewed Feb 19, 2019

View reviewed changes

b-admike reviewed Feb 21, 2019

View reviewed changes

raymondfeng reviewed Feb 21, 2019

View reviewed changes

docs/site/submitting_a_pr.md Show resolved Hide resolved

raymondfeng reviewed Feb 21, 2019

View reviewed changes

b-admike approved these changes Feb 21, 2019

View reviewed changes

raymondfeng reviewed Feb 21, 2019

View reviewed changes

raymondfeng reviewed Feb 25, 2019

View reviewed changes

dhmlau approved these changes Feb 25, 2019

View reviewed changes

raymondfeng reviewed Feb 25, 2019

View reviewed changes

raymondfeng approved these changes Feb 25, 2019

View reviewed changes

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

4824537

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

emonddr merged commit af4a55c into loopbackio:master Feb 26, 2019

emonddr deleted the emonddr_submit_pr_doc branch February 26, 2019 14:30

emonddr mentioned this pull request Feb 26, 2019

docs: link to 'submit_pr' tutorial loopbackio/loopback.io#816

Merged


		![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.


		Request a code review or document review.

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


		### 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).


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

		### 1. Log in to github

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


		### 7. Set up remote tracking


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


		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


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

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


		### 15. Rebasing

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


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


		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.


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

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


		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)


		### 6. Rebasing

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


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

		### 7. Squashing commits


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

		If you are a github expert, please follow the


		#### 2. Create the feature branch

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


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


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

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


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

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

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

Conversation

emonddr commented Feb 10, 2019 • edited Loading

emonddr commented Feb 10, 2019 • edited Loading

emonddr commented Feb 10, 2019

nabdelgadir left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

raymondfeng Feb 11, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emonddr Feb 12, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emonddr commented Feb 11, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

raymondfeng Feb 11, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dhmlau left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emonddr commented Feb 12, 2019

emonddr commented Feb 12, 2019

jannyHou left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emonddr commented Feb 13, 2019 • edited Loading

dhmlau commented Feb 13, 2019

bschrammIBM commented Feb 13, 2019 • edited Loading

raymondfeng commented Feb 14, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dhmlau left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emonddr commented Feb 10, 2019 •

edited

Loading

emonddr commented Feb 10, 2019 •

edited

Loading

raymondfeng Feb 11, 2019 •

edited

Loading

emonddr Feb 12, 2019 •

edited

Loading

raymondfeng Feb 11, 2019 •

edited

Loading

emonddr commented Feb 13, 2019 •

edited

Loading

bschrammIBM commented Feb 13, 2019 •

edited

Loading