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

Model association for sequalize-cli 4.0.0 updates #620

Merged
merged 4 commits into from
Feb 15, 2018

Conversation

buddylindsey
Copy link
Contributor

Created this to deal with a merge conflict so it can be merged. Preserved commits for contribution credit.

@buddylindsey buddylindsey changed the title Model association for sequalize-clie 4.0.0 updates Model association for sequalize-cli 4.0.0 updates Feb 14, 2018
@buddylindsey
Copy link
Contributor Author

@sushantdhiman I think the tests will pass if they can just be restarted. Looks like one got hung up for a very long time. Are you able to restart the build? I don't see a button to do it so I don't think I have permissions. If not I can try to find something to push to kick off another build.

@sushantdhiman
Copy link
Contributor

No problem, I will retry that test

@@ -197,7 +197,7 @@ const _ = require('lodash');

const targetContent = attrUnd.underscored ?
'underscored: true'
: '{\n classMethods';
: '{});';
Copy link
Contributor

Choose a reason for hiding this comment

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

assert associate

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry for being a bit dense. I am not sure what you are wanting.

Copy link
Contributor

Choose a reason for hiding this comment

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

We need to check if generated model contain associate method, just scanning file with .associate around here should be enough

Just add .pipe(helpers.ensureContent('.associate'))

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay. Will do.

@@ -210,7 +210,8 @@ const _ = require('lodash');
.src(Support.resolveSupportPath('tmp', 'models'))
.pipe(helpers.readFile('user.js'))
.pipe(helpers.ensureContent(targetContent))
.pipe(helpers.teardown(done));
.pipe(helpers.teardown(done))
.pipe(helpers.ensureContent('.associate'));
Copy link
Contributor

Choose a reason for hiding this comment

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

.pipe(helpers.ensureContent('.associate')), Should come before teardown, I think tests will fail

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hah. okay, they passed locally. Will move it. Totally didn't realize those where teardowns.

@buddylindsey
Copy link
Contributor Author

@sushantdhiman okay. I moved the line up above teardown and force pushed a rebase to keep the commit history a bit cleaner.

@buddylindsey
Copy link
Contributor Author

buddylindsey commented Feb 14, 2018

@sushantdhiman looks like we are good to go. It is now mergeable and the tests have all passed.

@sushantdhiman sushantdhiman merged commit 71770de into sequelize:master Feb 15, 2018
@buddylindsey buddylindsey deleted the model-association branch February 15, 2018 12:55
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