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

Added changes as requested for the new schematic support. #564

Merged
merged 3 commits into from Apr 24, 2013

Conversation

3 participants
@durango
Member

durango commented Apr 23, 2013

Will close the other PR... I had to redo my git base a bit :) Hence the new branch (from my side).

@durango

This comment has been minimized.

Member

durango commented Apr 23, 2013

Ah it took my other git commit as well for HSTORE, meh, @sdepold it's just basic HSTORE support let me know if it's OK to push if not I'll have to get a little crafty :/

whereCollection: null
whereCollection: null,
schema: null,
scheamDelimiter: ''

This comment has been minimized.

@sdepold

sdepold Apr 24, 2013

Member

typo alert :D

@durango

This comment has been minimized.

Member

durango commented Apr 24, 2013

Fixed :)

@sdepold sdepold merged commit 2cf9d4f into sequelize:master Apr 24, 2013

1 check passed

default The Travis build passed
Details
@kevinmartin

This comment has been minimized.

Contributor

kevinmartin commented on 38a1458 Apr 25, 2013

So, I assume the createSchema/dropSchema only works with PostgreSQL? The MySQL and SQLite code I see for those two methods only Show the tables within the schema...

This comment has been minimized.

Member

durango replied Apr 25, 2013

That is correct, there is no such thing in the MySQL world (even though its ANSI)

This comment has been minimized.

Contributor

kevinmartin replied Apr 27, 2013

Well 4 things then:

  1. In MySQL "database" = "schema", therefore CREATE SCHEMA is equivalent to CREATE DATABASE and vice-versa. So, why not add that as part of this feature?
  2. If not, then wouldn't it be better to pretty much return false; on the createSchema and dropSchema methods and have the DAO or whichever class is calling those methods detect if its false and, if so, run the callback instead of sending the querying over? This would prevent an unnecessary call to the database and therefore make it run faster.
  3. If not, then I think SELECT 1; is much faster than the queries you have up there.
  4. Also, SHOW TABLES; (and its SQLite equivalent) are pretty much retrieving data (GET), where the methods themselves are POST and DELETE, respectively. Doesn't make sense.

P.S. I would love to have CREATE DATABASE as a feature for MySQL. I am currently using that in my project and have to do it manualy using the Sequelize.prototype.query method;

This comment has been minimized.

Member

durango replied Apr 27, 2013

@kevinmartin:

I've already considered the following points throughout the development process.

  1. I was under the impression that sequelizejs could already do this for MySQL.

2 and 3. I've tried both tbh, for some reason the MySQL tests would hang with that rather than SELECT 1;, but either way I'll look into it.

  1. This is true, however, my primary concern is PostgreSQL, since uhm, that's a real database ;)

All in all, I'll take these considerations in and apply a patch.

This comment has been minimized.

Member

durango replied Apr 27, 2013

@kevinmartin I also just remembered, a lot of discussion came about this within the IRC channel, it was decided to make MySQL schemas a prefix for table names during that discussion (as a schema, is technically not suppose to be a new database). @sdepold any thoughts on this? I can implement actual database abstraction if you want or just change schema to database.

@sdepold

This comment has been minimized.

Member

sdepold commented Apr 29, 2013

just release 1.7.0-alpha1 which contains this feature

@durango durango deleted the durango:schema branch Jul 4, 2013

@durango durango referenced this pull request Jul 8, 2013

Closed

State of the Union #749

33 of 45 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment