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

Type warnings #3839

Merged
merged 1 commit into from Jun 2, 2015

Conversation

4 participants
@BridgeAR
Contributor

BridgeAR commented May 31, 2015

This is on top of #3836 and I wanted to get your feedback what you think about the implementation. So you only have to look at the "Initial commit" changes.

If someone uses a data type in a way that is not supported we'd now warn the user about it including what we do instead and presenting the link to the data types as they are in the current dialect.

@janmeier

This comment has been minimized.

Member

janmeier commented May 31, 2015

I like this implementation very much! :) LGTM

@BridgeAR

This comment has been minimized.

Contributor

BridgeAR commented May 31, 2015

This will close #2038

// MSSQL does not support any parameters for integer
if (this._length || this.options.length || this._unsigned || this._zerofill) {
this.warn('You used INTEGER with a attribute. This is not supported by MSSQL. Simple `INTEGER` will be used instead.');

This comment has been minimized.

@mickhansen

mickhansen Jun 1, 2015

Contributor

'an attribute'

ABSTRACT.prototype.warn = function(text) {
if (!warnings[text]) {
warnings[text] = true;
console.warn('>> WARNING:', text, '\n>> Check:', this.dialectTypes);

This comment has been minimized.

@mickhansen

mickhansen Jun 1, 2015

Contributor

Hmm, any way to have this include the actual line ala what depd does you think?

This comment has been minimized.

@janmeier

janmeier Jun 1, 2015

Member

Couldn't we actually just use depd here?

This comment has been minimized.

@mickhansen

mickhansen Jun 1, 2015

Contributor

It's not really a deprecation, i'm not sure if depd explicitely uses that wording though to.

This comment has been minimized.

@BridgeAR

BridgeAR Jun 1, 2015

Contributor

Hm that would be nice indeed... It seems like depd includes 'deprecated' hardcoded. I could include the colours without difficulties though :D

This comment has been minimized.

@mickhansen

mickhansen Jun 1, 2015

Contributor

Could duplicate the stack functionality though.

This comment has been minimized.

@BridgeAR

BridgeAR Jun 1, 2015

Contributor

I thought about it but let's move that to another day and another PR :)

this._unsigned = null;
this._zerofill = null;
if (this._length || this.options.length || this._unsigned || this._zerofill) {
this.warn('You used INTEGER with a parameter. This is not supported by POSTGRES. Simple `INTEGER` will be used instead.');

This comment has been minimized.

@mickhansen

mickhansen Jun 1, 2015

Contributor

Here it's 'parameter' the other places it's 'attribute', intentional?

This comment has been minimized.

@BridgeAR

BridgeAR Jun 1, 2015

Contributor

I'm unhappy with the wording but nothing better came to my mind. The difference I tried to emphasize is between BLOB('tiny') and INTEGER.UNSIGNED.

This comment has been minimized.

@mickhansen

mickhansen Jun 1, 2015

Contributor

I see, that difference isn't really obvious at all (to me atleast), so i'd say just use "parameter" for all.

This comment has been minimized.

@BridgeAR

BridgeAR Jun 1, 2015

Contributor

Done

@mickhansen

This comment has been minimized.

Contributor

mickhansen commented Jun 1, 2015

Looks great. Warnings are the way to go, since i don't personally thing we should ever remove support completely.

@BridgeAR

This comment has been minimized.

Contributor

BridgeAR commented Jun 1, 2015

Rebased, fixed a couple typos and included more warnings. Depending on what you wish I'd say it's ready to merge. We could provide the additional functionality to include the lines later on.

@mickhansen

This comment has been minimized.

Contributor

mickhansen commented Jun 1, 2015

You're still mixing attribute and parameter in the warnings, would be nice to streamline them. Or provide some reasoning why they are different.

@mickhansen

This comment has been minimized.

Contributor

mickhansen commented Jun 1, 2015

General wording might be better with something like 'postgres does not support TEXT with size. Simple 'TEXT' will be used instead`

@BridgeAR

This comment has been minimized.

Contributor

BridgeAR commented Jun 1, 2015

+1

@BridgeAR

This comment has been minimized.

Contributor

BridgeAR commented Jun 1, 2015

Changed the comments and rebased

mickhansen added a commit that referenced this pull request Jun 2, 2015

@mickhansen mickhansen merged commit 720d4d8 into sequelize:master Jun 2, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@diosney

This comment has been minimized.

Contributor

diosney commented Aug 5, 2016

Please, see #6385

I really don't like this PR, there is a way to disable completely this messages?

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