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

change(model): new options.underscored implementation #9304

Merged
merged 7 commits into from Apr 18, 2018
Merged

Conversation

@sushantdhiman
Copy link
Contributor

@sushantdhiman sushantdhiman commented Apr 14, 2018

Pull Request check-list

Please make sure to review and check all of these items:

  • Does npm run test or npm run test-DIALECT pass with this change (including linting)?
  • Does the description below contain a link to an existing issue (Closes #[issue]) or a description of the issue you are solving?
  • Have you added new tests to prevent regressions?
  • Is a documentation update included (if this change modifies existing APIs, or introduces new ones)?
  • Did you follow the commit message conventions explained in CONTRIBUTING.md?

Description of change

Implements proposal from #6423 (comment)

Closes #6423
Closes #7255

@sushantdhiman sushantdhiman requested review from janmeier, eseliger and sequelize/orm Apr 14, 2018
@sushantdhiman sushantdhiman mentioned this pull request Apr 14, 2018
5 of 5 tasks complete
@codecov
Copy link

@codecov codecov bot commented Apr 14, 2018

Codecov Report

Merging #9304 into master will increase coverage by 0.01%.
The diff coverage is 98.18%.

@eseliger
Copy link
Member

@eseliger eseliger commented Apr 14, 2018

looks good so far but this is a breaking change, isn't it?

@sushantdhiman
Copy link
Contributor Author

@sushantdhiman sushantdhiman commented Apr 14, 2018

We are under v5 beta process so we can break things

@eseliger
Copy link
Member

@eseliger eseliger commented Apr 14, 2018

oh I thought it was still the v5 branch

@@ -22,13 +22,14 @@ Player.belongsTo(Team); // Will add a teamId attribute to Player to hold the pri

By default the foreign key for a belongsTo relation will be generated from the target model name and the target primary key name.

The default casing is `camelCase` however if the source model is configured with `underscored: true` the foreignKey will be `snake_case`.
The default casing is `camelCase`. If the source model is configured with `underscored: true` the foreignKey will be created with field `snake_case`.

This comment has been minimized.

@janmeier

janmeier Apr 16, 2018
Member

Users might not know the field terminology yet - So maybe an explainer here

This comment has been minimized.

@janmeier

janmeier Apr 16, 2018
Member

I think this explainer should also be moved further up - It relates to all types of associations.

This comment has been minimized.

@sushantdhiman

sushantdhiman Apr 18, 2018
Author Contributor

There was section about foreign keys further down, I have moved that up and corrected old examples

@@ -191,7 +193,7 @@ const Project = sequelize.define('project', {/* ... */})
Project.hasMany(User, {as: 'Workers'})
```

This will add the attribute `projectId` or `project_id` to User. Instances of Project will get the accessors `getWorkers` and `setWorkers`.

This comment has been minimized.

@janmeier

janmeier Apr 16, 2018
Member

This will add the attribute projectId to the User model. Depending on your setting for underscored the column in the table will either be called projectId or project_id

@sushantdhiman sushantdhiman merged commit 5941bfe into master Apr 18, 2018
4 checks passed
4 checks passed
@codecov
codecov/patch 98.18% of diff hit (target 95.93%)
Details
@codecov
codecov/project 95.95% (+0.01%) compared to 3b12097
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@sushantdhiman sushantdhiman deleted the underscored branch Apr 18, 2018
@abelosorio
Copy link

@abelosorio abelosorio commented Apr 18, 2018

I'm super happy that this issue is closed! I was really anxious about this feature. Is there any idea on when (or which version) is this going to be released?

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

Successfully merging this pull request may close these issues.

None yet

4 participants