Skip to content

Conversation

@boazy
Copy link
Member

@boazy boazy commented Oct 29, 2016

@ronen could you please review this PR?

This all goes towards what we discussed in:
SchemaPlus/schema_plus_views#16

I ended up replacing the default Rails implementation, so perhaps it's better to move this code to a new, separate gem and use an implement Middleware.

boazy added 4 commits August 21, 2016 16:09
Middlewares can hook into Connection#data_sources and add their
own SQL WHERE constraints, e.g. to filter out tables.

The rails default implementation is overriden, but results should
be the same. Schema query implementation is unified for data sources
and views now.
@boazy
Copy link
Member Author

boazy commented Oct 29, 2016

Oh, and checks obviously fail because it relies on the new methods added to schema_plus_compatibility.

@ronen
Copy link
Member

ronen commented Nov 3, 2016

I ended up replacing the default Rails implementation, so perhaps it's better to move this code to a new, separate gem and use an implement Middleware.

Yeah I think it should be separate--schema_plus_core should stick to its philosophy of providing an API but not adding new features or changing implementation or behavior.

Only question is which gem should it go in... schema_plus_data_sources? Include it with an existing gem? I notice BTW that the existing schema_plus_tables has no use in AR 5.0; so schema_plus tables can be retired, or maybe it can become the home for this middleware?

@boazy
Copy link
Member Author

boazy commented Nov 4, 2016

This change actually doesn't affect default behavior, but it might do so in the future - if the rails implementation is changed.
But seeing how Rails 5 dealt with Connection#tables and Connection#data_sources, I don't expect they'll surprise us with a function that suddenly returns completely different results without having a deprecation warning first.

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.

3 participants