Skip to content

Rename tables for rails convention#328

Closed
BenMorganIO wants to merge 8 commits into
solidusio:masterfrom
BenMorganIO:rename-tables-for-rails-convention
Closed

Rename tables for rails convention#328
BenMorganIO wants to merge 8 commits into
solidusio:masterfrom
BenMorganIO:rename-tables-for-rails-convention

Conversation

@BenMorganIO

Copy link
Copy Markdown
Contributor

No description provided.

@BenMorganIO BenMorganIO mentioned this pull request Aug 26, 2015
7 tasks
@magnusvk

Copy link
Copy Markdown
Contributor

This looks good, though I'd want to split up the migration. As this stands, the whole migration will run in one database migration which feels dangerous and I don't think is necessary. If we had one migration per rename, it would run each in its own transaction.

@BenMorganIO

Copy link
Copy Markdown
Contributor Author

@magnusvk I think this will be the last time I separate things out...

@magnusvk

Copy link
Copy Markdown
Contributor

:)

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.

Using DISTINCT that way, will not work in PostgreSQL (if I undestand correctly, why distinct is used there). In documentation, we have example:

SELECT DISTINCT ON (location) location, time, report
    FROM weather_reports
    ORDER BY location, time DESC;

So when we want to show records with unique spree_product.id then we need to add ON before field name, but this is not compatible with other DBs.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@grzlus If we add the ON keyword, then we could definitely move this to Arel, but you're right, this won't be compatible with other DBs. I think I'll just make an if statement for database type. Its not nice, but it'll do :(

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.

This will not be so easy: https://github.com/rails/arel/blob/master/lib/arel/visitors/to_sql.rb#L298 Only PostgreSQL has implemented visit function for distinct_on

You need to guess what DB is used and use distinct/distinct on 😞

@cbrunsdon

Copy link
Copy Markdown
Contributor

Cleaning up these commits isn't going to be worth the effort to get them into core, the majority of the important functionality of #289 already got in, so just killing this.

@cbrunsdon cbrunsdon closed this Nov 25, 2015
@BenMorganIO BenMorganIO deleted the rename-tables-for-rails-convention branch November 25, 2015 23:49
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