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 middleware; include schema definitions in dump; minor cleanups #2

Merged
merged 11 commits into from
Aug 29, 2015

Conversation

ronen
Copy link
Member

@ronen ronen commented Aug 28, 2015

@stenver Here's an (awful big, sorry) PR right back at you. It's a bunch of mostly small commits, but since they mostly all depend on each other I didn't want to make them individual PRs. Here's the details:

c98f4ee remove ‘pry’ dependency:

I do use pry myself, but don't want to force all developers to use it

51584b7 include schema_plus_foreign_keys development dependency:

I needed to do that for the tests to run.

6f3f4e4 move tables implementation into middleware:

This was my bad: I had suggested modifying the connection adapter so that tables would return the schema-prefixed table names, which you did. But I had forgotten that schema_plus_core already defined a middleware stack for tables, which is the cleaner way to do it. So I moved your code over to be middleware.

e2e4a8a Simplify query code.

Once I was in there touching the query code, I noticed it was following boilerplate from ActiveRecord's PostgreSQL adapter, that handles arel queries -- but that was overkill in this case since the SQL is hardwired. So I simplified it a bit.

dc71f75 move suppress_messages into spec_helper:

A minor cleanup I've been doing gradually in the other SchemaPlus gems but figured I might as well do here as long as I'm at it.

ef79d64 Spec cleanup: add (multi)schema support methods:

Anticipating other other cleanups and new examples, I added spec/support/schema.rb that defines some helpers. In particular it defines with_schemas(['first', 'second']) do ... which encapsulates creating schemas, setting the search path, executing the code, then cleaning everything up -- removing those namespaces and their tables.

(OK I didn't really anticipate, I did this after the fact but then shuffled around the commits :)

7e972bc Spec cleanup: Make separate schema definitions for each context rather than one global schema for all examples:

You followed a pattern that's common in other schema_plus gem spec suites: make one big honkin' schema definition with every case in it, then have individual examples to verify the various features. But those spec suites are mostly legacy from a long time ago, and I haven't been through to clean them all up.

I think cleaner is to make a separate schema definition for each context, in order to test features individually. That makes the relationship between the schema and the tests more obvious, and makes the entire suite less fragile -- with one big schema definition, if you need something new for a new test you risk breaking the old tests by messing with the schema.

a759df7 Add spec for connection.tables

You originally wanted to do this in order to fix the dump; but the solution was to change connection.tables. And that change is an appropriate component of supporting multiple schemas. But it seemed to me that the new behavior of connection.tables should have its own test suite.

651d4b5 don’t add prefix when using default schema path

This is something that we didn't discuss. But a general goal for the schema_plus family of gems is that if their features aren't used, including them should be a no-op. So I thought this gem should suppress adding a prefix if the current schema path is PostgreSQL's default.

Does that seem OK to you?

3ab67a8 Added schema creation and search path

It occurred to me that just including the schema prefix in the dump makes a dump that's not really usable, since you couldn't do rake db:setup as the schemas wouldn't be defined. So (using more middleware) I included the CREATE SCHEMA IF NOT EXISTS and schema_search_path = statements at the top of the dump.

Does that also seem OK to you?

fb9ee2c Update doc to describe change to connection.tables, and including the schema setup in the dump.

Brought the README in line with all the above.

ronen added 11 commits August 28, 2015 21:46
No need to go through arel and bindings since it’s hardwired SQL; use higher-level connection.exec_query method for simpler API
In particular, with_schemas([names]) is a wrapper creates the schemas and sets the search path, and cleans up after itself:  removes all tables in those schemas as well as the schema definitions themselves
@stenver
Copy link
Contributor

stenver commented Aug 29, 2015

Thanks @ronen - very awesome that you took the time to make this massive improvement to this gem. Also thanks for the very well documented changes on commits - they were super helpful in understanding your thought process.

don’t add prefix when using default schema path
I think its fine - after all, if you are not using multiple schemas, then none of the features of this gem are required. It will also keep the code cleaner for the users of single schema.

3ab67a8 Added schema creation and search path
This is really my bad. Thanks for fixing it. When I was testing it out in another project, I never did rake db:setup, since I cannot use schema plus dump for database setup in there yet - we have custom types in our schema. I will make sure to create a mini project to properly test it out on the next time.

I have a few comments about the readme, but I can make it as a separate PR so it wouldnt hold back this person: SchemaPlus/schema_plus_views#10 (comment)

stenver added a commit that referenced this pull request Aug 29, 2015
Use middleware; include schema definitions in dump; minor cleanups
@stenver stenver merged commit c397bac into master Aug 29, 2015
@stenver stenver deleted the middleware branch August 29, 2015 06:25
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