Skip to content

allow additional options to be specified for associations #42

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

Merged

Conversation

grbsk
Copy link
Contributor

@grbsk grbsk commented Jul 6, 2017

Affects BelongsTo, BelongsToMany, HasMany, HasOne, and ForeignKey attributes. Compatibility with existing parameter that takes string name of foreign keys has been provided by letting the foreign key be a string (existing functionality) or the appropriate association options (new functionality). Additionally, ForeignKey can now receive options. By default it simply sets the foreign key name to the property name, which should be backwards compatible.

I did not add tests for the Many-to-Many cases, because additional options outside setting the FK column names and table name are mostly superfluous.

Fixes #34

@codecov-io
Copy link

codecov-io commented Jul 6, 2017

Codecov Report

Merging #42 into master will increase coverage by 0.16%.
The diff coverage is 92.85%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #42      +/-   ##
==========================================
+ Coverage    94.5%   94.67%   +0.16%     
==========================================
  Files          65       65              
  Lines         692      713      +21     
  Branches       91       99       +8     
==========================================
+ Hits          654      675      +21     
  Misses         14       14              
  Partials       24       24
Impacted Files Coverage Δ
lib/annotations/ForeignKey.ts 100% <100%> (ø) ⬆️
lib/models/BaseSequelize.ts 88.33% <100%> (+0.4%) ⬆️
lib/annotations/association/HasMany.ts 100% <100%> (ø) ⬆️
lib/annotations/association/HasOne.ts 100% <100%> (ø) ⬆️
lib/annotations/association/BelongsToMany.ts 100% <100%> (ø) ⬆️
lib/annotations/association/BelongsTo.ts 100% <100%> (ø) ⬆️
lib/services/association.ts 86.44% <80%> (+1.82%) ⬆️

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 76b2920...8fc9138. Read the comment docs.

@RobinBuschmann
Copy link
Member

@hackedbychinese thanks for implementing this. Looks good.

codecov/patch — 92.85% of diff hit (target 94.5%)

is not an issue. I will merge it anyway

@RobinBuschmann RobinBuschmann merged commit f25f2c9 into sequelize:master Jul 6, 2017
@grbsk grbsk deleted the feature/issue-34-association-options branch July 6, 2017 17:47
@grbsk
Copy link
Contributor Author

grbsk commented Jul 8, 2017

Hello! Any comment on when we might see this in a release? Thanks!

@RobinBuschmann
Copy link
Member

Hey, I'm planning to release a new version at the end of this weekend or not later than next week. To make this happen I need to fix #43 and #37 .

@RobinBuschmann
Copy link
Member

@hackedbychinese Hey, I removed the AssociationForeignKeyOptions from the ForeignKey annotation, because these options only consists of a name and ColumnOptions. These options should rather be set via Column annotation, which is already part of sequelize-typescript.

So that the following example, which shows your implementation..

@ForeignKey(() => Model, {name: 'fooId', field: 'foo_id'})
fooId: number;

..should be achieved with

@ForeignKey(() => Model)
@Column({field: 'foo_id'})
fooId: number;

I think that this approach makes the api much cleaner. What do you think?

Additionally I added support for otherKey in the AssociationOptions of BelongsToMany.

@grbsk
Copy link
Contributor Author

grbsk commented Jul 15, 2017

Sure, that makes sense. Thanks!

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.

3 participants