Skip to content

Fix correct previous value#7186

Merged
sushantdhiman merged 1 commit intosequelize:v3from
nemisj:fix-previous
Feb 4, 2017
Merged

Fix correct previous value#7186
sushantdhiman merged 1 commit intosequelize:v3from
nemisj:fix-previous

Conversation

@nemisj
Copy link
Copy Markdown

@nemisj nemisj commented Feb 2, 2017

Pull Request check-list

  • Does npm run test or npm run test-DIALECT pass with this change (including linting)?
  • Does your issue contain a link to 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)?
  • Have you added an entry under Future in the changelog?

Description of change

Previous method of the instance was working incorrectly

@mention-bot
Copy link
Copy Markdown

@nemisj, thanks for your PR! By analyzing the history of the files in this pull request, we identified @mickhansen, @BridgeAR and @janmeier to be potential reviewers.


describe(Support.getTestDialectTeaser('Instance'), function() {
describe('previous', function () {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

No extra line

var Model = current.define('Model', {
text: {
type: DataTypes.STRING
, get: function (name) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

no leading commas, in json object

@sushantdhiman
Copy link
Copy Markdown
Contributor

@nemisj Need a PR to master before merging this

@codecov-io
Copy link
Copy Markdown

codecov-io commented Feb 3, 2017

Codecov Report

❗ No coverage uploaded for pull request base (v3@6b597dc). Click here to learn what that means.


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 6b597dc...9927fb9. Read the comment docs.

@nemisj
Copy link
Copy Markdown
Author

nemisj commented Feb 3, 2017

@sushantdhiman what do you mean? do I have to create the same PR for master branch?

@sushantdhiman
Copy link
Copy Markdown
Contributor

what do you mean? do I have to create the same PR for master branch?

Yes, add same PR to master as well @nemisj

@nemisj
Copy link
Copy Markdown
Author

nemisj commented Feb 3, 2017

@sushantdhiman i've added it - #7189

Copy link
Copy Markdown
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.

Needs changelog, otherwise LGTM

@nemisj
Copy link
Copy Markdown
Author

nemisj commented Feb 3, 2017

@sushantdhiman how should this happen? Do I have to add it to this branch and then you will merge it?

@sushantdhiman
Copy link
Copy Markdown
Contributor

Yes, you need to add changelog

@nemisj
Copy link
Copy Markdown
Author

nemisj commented Feb 3, 2017

@sushantdhiman, added to both PRs

changelog.md Outdated
@@ -1,3 +1,6 @@
# 3.30.2
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Future

, DataTypes = require(__dirname + '/../../../lib/data-types')
, current = Support.sequelize;

describe(Support.getTestDialectTeaser('Instance'), function() {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Space before opening brackets function ()

@nemisj
Copy link
Copy Markdown
Author

nemisj commented Feb 4, 2017

done

@sushantdhiman sushantdhiman merged commit ab0570d into sequelize:v3 Feb 4, 2017
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.

4 participants