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

adding options on refresh() method (Fixing issue #649) #650

Closed
wants to merge 8 commits into from
Closed

adding options on refresh() method (Fixing issue #649) #650

wants to merge 8 commits into from

Conversation

urishabh11
Copy link
Contributor

@urishabh11 urishabh11 commented Feb 11, 2021

Proposed changes

Fixing issue #649

Making refresh method use transaction.

Types of changes

  • Bugfix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist

  • I have read the CONTRIBUTING doc
  • Lint and unit tests pass locally with my changes
  • I have added tests that prove my fix is effective or that my feature works.
  • I have added necessary documentation (if appropriate)

@urishabh11 urishabh11 marked this pull request as ready for review February 11, 2021 12:43
Copy link
Member

@thetutlage thetutlage left a comment

Choose a reason for hiding this comment

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

Also, can you please add tests for the changes you have made?

src/Orm/BaseModel/index.ts Outdated Show resolved Hide resolved
.eslintrc.json Outdated Show resolved Hide resolved
Copy link
Contributor Author

@urishabh11 urishabh11 left a comment

Choose a reason for hiding this comment

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

Requested changes updated.

.eslintrc.json Outdated Show resolved Hide resolved
This reverts commit 525c89e.
test/orm/base-model.spec.ts Outdated Show resolved Hide resolved
src/Orm/BaseModel/index.ts Outdated Show resolved Hide resolved
Co-authored-by: Romain Lanz <2793951+RomainLanz@users.noreply.github.com>
Copy link
Contributor Author

@urishabh11 urishabh11 left a comment

Choose a reason for hiding this comment

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

sorry for the typo of commit description, it should be no-shadow

@urishabh11
Copy link
Contributor Author

@thetutlage

please let me know if I should do any changes.

@RomainLanz
Copy link
Member

Hey !

It seems that you added few changes that are not related to this PR, also, there's some conflicts, could you fix them?

@urishabh11
Copy link
Contributor Author

@RomainLanz

Yes,

  1. I can fix the conflicts.
  2. The changes which are not related to the PR, are about ESLINT error. Should I reverse that as well?
  3. If you suggest I can create a new PR with the final changes.

@thetutlage
Copy link
Member

Hello @urishabh11

Thanks for taking out time to work on the PR. The changes required a bit more code to make it work properly. To be precise, I have moved the refresh logic to the ModelAdapter, as it handles all the actions for a given model instance 7a37e08#diff-ecdf535c4ff3c6a527f2a33ea87a1777a189d248368d0febc66f6bdb01d0c9e7R90-R108

@urishabh11
Copy link
Contributor Author

@thetutlage

That's great 👍

Waiting for the new release with the updates :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants