Skip to content

MorphMany#111

Open
ryandialpad wants to merge 23 commits intomasterfrom
morph-many
Open

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

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

@ryandialpad ryandialpad Nov 25, 2021

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?

@ryandialpad ryandialpad requested a review from cuebit November 29, 2021 17:52
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

mt-gareth commented Mar 16, 2022

Can we push a new release with this in it?

@mrvanwagoner
Copy link

mrvanwagoner commented Mar 19, 2022

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