Skip to content
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

Use ActiveRecord methods to generate INSERT SQLs #26

Merged
merged 2 commits into from
Dec 7, 2015

Conversation

dmnelson
Copy link
Contributor

@dmnelson dmnelson commented Dec 7, 2015

I was trying to use Polo with PostgreSQL but ran into couple of issues (I using it from Master with the PR related to Postgres that was merged couple of days ago):

To get that fixed I went ahead and changed the code to use ActiveRecord methods to generate SQL, and after checking it manually, I could see it working fine for both PostgreSQL and MySQL.

The reason why I had to change all SQLs on the specs to be double-quoted is because that is the standard for SQLite, as it only supports backticks to the compatible with MySQL (see https://www.sqlite.org/lang_keywords.html)

I personally believe that these changes really need the work on #23 to be done. And additionally it would be nice to test DBMS specifics, like the different data types each one has (e.g. hstore, json or array on PostgreSQL) and different syntaxes. But I'm not sure what would be the best practices to structure and organize those tests.

values.each do |attribute, value|
column = record.column_for_attribute(attribute.name)
values[attribute] = connection.type_cast(value, column)
end
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wish this block could be avoided and have that done automatically, but I couldn't figure out how.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it looks alright. It's ok since this code is extracted into its own method.
It might be worth writing some docs for this method/module now that things are getting a little more complicated.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@nettofarah Done! Let me know if it looks alright.

Copy link
Contributor

Choose a reason for hiding this comment

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

This is fantastic.

@nettofarah
Copy link
Contributor

hey, @dmnelson.
Thank you so much for submitting this. It is looking good!

Yeah, I'm not sure if we should hold off until we get #23 sorted out. Though I could see how this would help other people going forward.

I'll see if I can look into #23 this week so we can unblock some other features.

@nettofarah
Copy link
Contributor

I think it is safe to merge this in since the tests are passing and it is well documented.
Ideally, we should get #23 out, but I think we should be fine for now, since #23 is a more involved change anyway.

Thank you, @dmnelson!
Would you mind taking our survey on #19?

nettofarah added a commit that referenced this pull request Dec 7, 2015
Use ActiveRecord methods to generate INSERT SQLs
@nettofarah nettofarah merged commit f563d92 into IFTTT:master Dec 7, 2015
@dmnelson dmnelson mentioned this pull request Mar 23, 2016
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.

2 participants