Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

MorphMany #111

Open
wants to merge 23 commits into
base: master
Choose a base branch
from
Open

MorphMany #111

wants to merge 23 commits into from

Conversation

ryandialpad
Copy link
Contributor

@ryandialpad ryandialpad commented Nov 25, 2021 •
edited
Loading

Type of PR:

  • Bugfix
  • Feature
  • Refactor
  • Code style update
  • Build-related changes
  • Test
  • Documentation
  • Other, please describe:

Breaking changes:

  • No
  • Yes

Details

Implements MorphMany relation

Will rebase when MorphTo is merged into master #106

@ryandialpad ryandialpad self-assigned this Nov 25, 2021
@codecov
Copy link

codecov bot commented Nov 25, 2021 •
edited
Loading

Codecov Report

Merging #111 (f16c621) into master (6c23c18) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##            master      #111   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           40        42    +2     
  Lines         1251      1293   +42     
  Branches       185       192    +7     
=========================================
+ Hits          1251      1293   +42
Impacted Files Coverage Δ
src/model/Model.ts 100.00% <100.00%> (ø)
src/model/attributes/relations/MorphMany.ts 100.00% <100.00%> (ø)
...model/decorators/attributes/relations/MorphMany.ts 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6c23c18...f16c621. Read the comment docs.

ryandialpad
ryandialpad commented Nov 25, 2021
View reviewed changes
src/model/attributes/relations/MorphMany.ts
import { Model } from '../../Model'
import { Relation, Dictionary } from './Relation'

export class MorphMany extends Relation {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am noticing a fair amount of code duplication, at some point we might want to DRY these up once the library is feature complete (1.x).

@ryandialpad ryandialpad marked this pull request as ready for review November 25, 2021 19:11
cuebit
cuebit reviewed Nov 28, 2021
View reviewed changes
Copy link
Member

@cuebit cuebit left a comment

Choose a reason for hiding this comment

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

A few snags but looks good. Great work.

Additionally, could we add the relevant exports for the relation to index.cjs.ts?

test/feature/relations/morph_many_retrieve.spec.ts Outdated Show resolved Hide resolved
test/feature/relations/morph_many_retrieve.spec.ts Outdated Show resolved Hide resolved
test/unit/model/Model_Relations.spec.ts Outdated Show resolved Hide resolved
cuebit
cuebit approved these changes Nov 29, 2021
View reviewed changes
Copy link
Member

@cuebit cuebit left a comment

Choose a reason for hiding this comment

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

🎉🎉🎉

@cuebit cuebit added the enhancement New feature or request label Nov 29, 2021
@kiaking
Copy link
Member

kiaking commented Dec 3, 2021

@ryandialpad Sorry it took long! MorphTo is merged. Could you rebase this one 🙏

@ryandialpad
Copy link
Contributor Author

ryandialpad commented Dec 3, 2021 •
edited
Loading

@ryandialpad Sorry it took long! MorphTo is merged. Could you rebase this one 🙏

No worries! Rebased 😃

@mt-gareth
Copy link

Can we push a new release with this in it?

@mrvanwagoner
Copy link

Can we push a new release with this in it?

What is the timeframe to get this pushed? I am VERY anxious to be able to use this!

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

@cuebit cuebit cuebit approved these changes

@kiaking kiaking Awaiting requested review from kiaking

Assignees

@ryandialpad ryandialpad

Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@ryandialpad @kiaking @mt-gareth @mrvanwagoner @cuebit