Skip to content

Conversation

@haotangio
Copy link
Contributor

@haotangio haotangio commented Dec 26, 2018

PR for #2118 Update document for recursive belongsTo relationship.

Fixes #2118

Checklist

  • npm test passes on your machine
  • New tests added or existing tests modified to cover all changes
  • Code conforms with the style guide
  • API Documentation in code was updated
  • Documentation in /docs/site was updated
  • Affected artifact templates in packages/cli were updated
  • Affected example projects in examples/* were updated

Copy link
Member

@bajtos bajtos left a comment

Choose a reason for hiding this comment

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

Hi @haotangio, thank you for the pull request!

The changes looks mostly good (see my two minor comments below).

Unfortunately the CI build is failing, please take a look.

  • I suspect the "TASK=test" failure are caused by temporary unavailability of a Calculator SOAP service, they should go away when the build is re-run.

  • The new markdown content you are adding is not following our formatting rules, see https://travis-ci.org/strongloop/loopback-next/jobs/472240748:

    > loopback-next@0.1.0 prettier:cli /home/travis/build/strongloop/loopback-next
    > node packages/build/bin/run-prettier "**/*.ts" "**/*.js" "**/*.md" "-l"
    docs/site/BelongsTo-relation.md
    

    Please run npm run lint:fix and then git commit --amend to fix that.

@bajtos bajtos added Docs Relations Model relations (has many, etc.) feature labels Jan 7, 2019
@haotangio
Copy link
Contributor Author

Hello @bajtos.

Thank you for very much for your feedback on my first PR on loopback.
I follow your comment to fix the document and pushed new code. Please help me check it.

Btw, I think if we have a githook like precommit: npm run lint or precommit: npm run test, that would be very useful for contributors to avoid lint/test issues.

@bajtos
Copy link
Member

bajtos commented Jan 8, 2019

Btw, I think if we have a githook like precommit: npm run lint or precommit: npm run test, that would be very useful for contributors to avoid lint/test issues.

Thank you for the suggestion. We already have a commit hook to check the commit message.

Personally, I am reluctant to run tests & linting at every commit, because tasks like rebasing would become super slow.

Copy link
Member

@bajtos bajtos left a comment

Choose a reason for hiding this comment

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

LGTM.

Before we can land this patch, we need you @haotangio to sign our CLA here: https://cla.strongloop.com/agreements/strongloop/loopback-next

@bajtos bajtos self-assigned this Jan 8, 2019
@haotangio
Copy link
Contributor Author

@bajtos I signed the CLA. Thank you :)

Copy link
Contributor

@b-admike b-admike left a comment

Choose a reason for hiding this comment

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

Thank you for your contribution @haotangio.

@bajtos
Copy link
Member

bajtos commented Jan 10, 2019

I signed the CLA. Thank you :)

Thank you! I see that the first commit has been authored by @haotangio and commited by @haotcdgroup. We will need both handles to sign our CLA before we can proceed.

Assuming both those handles belong to you, you can also squash the commits into a single one and make both the author and the committer to be the single person, the handle which you used to sign our CLA with.

I apologize for the inconvenience this may create.

@haotangio
Copy link
Contributor Author

haotangio commented Jan 11, 2019

@bajtos Thank you for informing me about that.
I squashed the commits and pushed it with single one under my correct account.

@bajtos
Copy link
Member

bajtos commented Jan 11, 2019

Hmm, the build is failing on Travis CI because of timeouts, let's fix the tests first - see #2238

@bajtos
Copy link
Member

bajtos commented Jan 11, 2019

Rebased on top of the latest master. Let's wait for CI results, hopefully all will be green now.

@bajtos bajtos merged commit 9cc6e27 into loopbackio:master Jan 11, 2019
@bajtos
Copy link
Member

bajtos commented Jan 11, 2019

Landed, thank you @haotangio for the contribution!

@haotangio
Copy link
Contributor Author

Thank you @bajtos
Hope that I and my team could contribute more to Loopback 4 not only document but also code related part in the future :)

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

Labels

Docs feature Relations Model relations (has many, etc.)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants