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

Rename Instance private options and __options #4429

Merged
merged 1 commit into from Sep 9, 2015

Conversation

3 participants
@andywhite37
Contributor

andywhite37 commented Sep 3, 2015

  • Possibly fix #4423
  • Rename Instance options to $options to prevent value
    overwriting with models that have a column named options
  • Also rename Instance __options to $modelOptions to avoid confusion
    between $options and __options.
Rename Instance private options and __options
- Possibly fix #4423
- Rename Instance `options` to `$options` to prevent value
  overwriting with models that have a column named `options`
- Also rename Instance `__options` to `$modelOptions` to avoid confusion
  between $options and __options.
@janmeier

This comment has been minimized.

Member

janmeier commented Sep 9, 2015

Looks good to me - good catch on the __options - model options is definitely a better name

This shouldn't require a major version right @mickhansen ? Do you agree that if people depend on .options its their own fault ;) ?

@mickhansen

This comment has been minimized.

Contributor

mickhansen commented Sep 9, 2015

It's not documented anywhere, so can hardly be considered public API. I personally probably have some code dependant on that somewhere, but yeah i'd agree that if people are using non-public stuff they can't expect SEMVER to apply there.

@janmeier

This comment has been minimized.

Member

janmeier commented Sep 9, 2015

Cool, I'll merge and add it to the changelog then

janmeier added a commit that referenced this pull request Sep 9, 2015

Merge pull request #4429 from andywhite37/master
Rename Instance private options and __options

@janmeier janmeier merged commit 3c0d233 into sequelize:master Sep 9, 2015

janmeier added a commit to janmeier/sequelize that referenced this pull request Sep 9, 2015

janmeier added a commit that referenced this pull request Sep 9, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment