Skip to content

MorphMany#111

Open
ryandialpad wants to merge 23 commits into
masterfrom
morph-many
Open

MorphMany#111
ryandialpad wants to merge 23 commits into
masterfrom
morph-many

Conversation

@ryandialpad

@ryandialpad ryandialpad commented Nov 25, 2021 •
edited
Loading

Copy link
Copy Markdown
Contributor

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

codecov Bot commented Nov 25, 2021 •
edited
Loading

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 100.00%. Comparing base (6c23c18) to head (f16c621).

Additional details and impacted files 
@@            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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

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

export class MorphMany extends Relation {

@ryandialpad ryandialpad Nov 25, 2021

Copy link
Copy Markdown
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

@cuebit cuebit left a comment

Copy link
Copy Markdown
Member

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?

Comment thread test/feature/relations/morph_many_retrieve.spec.ts Outdated
Comment thread test/feature/relations/morph_many_retrieve.spec.ts Outdated
Comment thread test/unit/model/Model_Relations.spec.ts Outdated
@ryandialpad
ryandialpad requested a review from cuebit November 29, 2021 17:52
cuebit
cuebit approved these changes Nov 29, 2021
View reviewed changes

@cuebit cuebit left a comment

Copy link
Copy Markdown
Member

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

kiaking commented Dec 3, 2021

Copy link
Copy Markdown
Member

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

@ryandialpad

ryandialpad commented Dec 3, 2021 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

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

No worries! Rebased 😃

@mt-gareth

mt-gareth commented Mar 16, 2022

Copy link
Copy Markdown

Can we push a new release with this in it?

@mrvanwagoner

mrvanwagoner commented Mar 19, 2022

Copy link
Copy Markdown

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