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 drop of multiple columns in sqlite3 #2107

Merged
merged 1 commit into from Jul 25, 2017

Conversation

Projects
None yet
2 participants
@rizip1
Contributor

rizip1 commented Jun 5, 2017

  • Bugfix for issue #1979
  • The problem was that dropColumn method in sqlite3
    Compiler was using only first argument it got from
    apply
  • Reworked to use 'arguments' instead
Fix drop of multiple columns in sqlite3
* Bugfix for issue #1979
* The problem was that dropColumn method in sqlite3
  Compiler was using only first argument it got from
  apply
* Reworked to use 'arguments' instead
@elhigu

This comment has been minimized.

Collaborator

elhigu commented Jun 9, 2017

Documentation says that dropColumn drops just one column and dropColumns is for multiple columns. Am I missing something here?

http://knexjs.org/#Schema-dropColumn
http://knexjs.org/#Schema-dropColumns

@rizip1

This comment has been minimized.

Contributor

rizip1 commented Jun 10, 2017

Yes you are right. The problem in sqlite3 is that dropColumns will drop only first column. So I made it work for multiple columns. But the way I fixed it dropColumn now behaves same as dropColumns what may not be desired so can rework this.

@elhigu

This comment has been minimized.

Collaborator

elhigu commented Jun 10, 2017

I have no idea why there are 2 separate methods for dropping columns, but since they are already there, better to keep functionality as it is documented.

@rizip1

This comment has been minimized.

Contributor

rizip1 commented Jun 11, 2017

I have looked on the behavior of dropColumn and dropColumns and I think that they do the same. When you look into schema/builder/tablebuilder.js file you can see this piece of code

AlterMethods.dropColumn =
AlterMethods.dropColumns = function() {
  this._statements.push({
    grouping: 'alterTable',
    method: 'dropColumn',
    args: toArray(arguments)
  });
  return this;
};

So dropColumn is in fact dropColumns. I tried it also in postgresql and you can drop multiple columns just using dropColumn function. From that point of view dropColumn is really useless because one can just use dropColumns also for single column.
So to keep functionality as documented in sqlite3 a would also have to change behavior for other dialects (this function is common for all dialects, so it implies that even now the functionality does now follow the documentation).
Would not it be better to take the fix as it is because anyway this strange behavior is common for other dialects just for sqlite3 it does not work now. And maybe do the refactoring in separate PR?

@elhigu

This comment has been minimized.

Collaborator

elhigu commented Jun 26, 2017

@rizip1 sounds legit. Would be good to also fix documentation about this to tell that functions are actually the same. Was there already integration test for this functionality (dropColumn / dropColumns)?

@rizip1

This comment has been minimized.

Contributor

rizip1 commented Jul 2, 2017

Currently there is no integration test for dropColumns but there are some for dropColumn. Not sure if changing documentation is good way to go as knowing that dropColumn will also drop multiple columns could lead to complications later if someone starts to use it that way and the functionality changes in the future.

@elhigu elhigu merged commit 822f3d3 into tgriesser:master Jul 25, 2017

1 check passed

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

This comment has been minimized.

Collaborator

elhigu commented Jul 25, 2017

thanks 👍

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