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

fix(dropSchema): missing mssql implementation #9081

Merged
merged 5 commits into from
Feb 28, 2018

Conversation

jerfowler
Copy link
Contributor

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

This implements dropSchema for MSSQL.

Closes #9080

@eseliger
Copy link
Member

@jerfowler The test is failing

Drops all tables and constraints associated with a MSSQL schema

Closes sequelize#9080
@codecov
Copy link

codecov bot commented Feb 21, 2018

Codecov Report

Merging #9081 into master will increase coverage by <.01%.
The diff coverage is 100%.

Added top 1/order by to the guarantee constraints are dropped first. Also added more tests to
add/drop schemas with constraints.
Fixing issue with prior commit that broke platforms that don't natively support schemas
Copy link
Contributor

@sushantdhiman sushantdhiman left a comment

Choose a reason for hiding this comment

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

Test code use self = this approach, with arrow function we don't need that anymore. Please fix those tests. Otherwise this is good to go

@jerfowler
Copy link
Contributor Author

Sorry, @sushantdhiman, I'm not sure I follow what you're asking for. Am I not supposed to use arrow functions in test code? Can you give an example of the proper approach test code should follow?

@sushantdhiman
Copy link
Contributor

@jerfowler With arrow function we dont need to keep reference of parent scope this (like https://github.com/sequelize/sequelize/pull/9081/files#diff-ded6a833f0f1a8a3eb7ccf8042816afbR329)

So keep using arrow functions but get rid of self, use this directly instead

…riable

Using arrow functions we don't need to keep track of the parent `this` scope.
@jerfowler
Copy link
Contributor Author

Not sure why that sqlite test failed... nothing I did. Can we force a retest and see if it clears?

@jerfowler jerfowler closed this Feb 27, 2018
@jerfowler jerfowler reopened this Feb 27, 2018
@jerfowler
Copy link
Contributor Author

Closing the Pull request and Reopening worked, all tests pass! woohoo!

@sushantdhiman sushantdhiman requested a review from a team February 28, 2018 05: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.

MSSQL dropSchema doesn't work - Abstract Defaults to dropTableQuery?
3 participants