Skip to content
This repository has been archived by the owner on May 13, 2022. It is now read-only.

Refactoring for 2.0 #197

Closed
ronen opened this issue Jan 17, 2015 · 23 comments
Closed

Refactoring for 2.0 #197

ronen opened this issue Jan 17, 2015 · 23 comments

Comments

@ronen
Copy link
Member

ronen commented Jan 17, 2015

Hi all, I'm putting this out for feedback before I start creating new gems.

The refactor branch is coming along nicely, roughly along the lines proposed in #168 and #188 . Below the line I've listed all the new gems. (They are all currently living under 'lib/')

I'm open to suggestions for better gem names, for different partitioning of the functionality, anything else...?


  • schema_monkey

    This monkey-patches AR to create a bunch of internal-api hooks, using a 'middleware' style, thanks to the mitchellh/middleware gem. Not everything can currently be done using the middleware hooks; but quite a fair bit can. And I'm continuing to update schema_monkey as this refactor goes on and I observe new patterns. The goal is that a gem might need to add new methods to AR classes, but shouldn't (a) redefine existing AR methods nor (b) call AR internal methods. It's getting pretty close.

    It also does a bunch of configuration based on convention; so each individual feature gem only needs to do SchemaMonkey.register(FeatureGemModule) and everything gets set up and inserted into rails automatically.

    (Sorry, not documented yet.)

  • schema_db_default

Adds the ability to do record.update_attributes(attr: ActiveRecord::DB_DEFAULT) to update the attribute to the default value specified in the db schema.

  • schema_default_expr

    Adds the ability to specify an SQL expression for t.column :foo, default: { expr: "an SQL expression" }, and also a shorthand default: :now for timestamp columns. [Or does AR4.2 already support default expressions?]

  • schema_index_plus.rb

    [Better name?] Adds shorthands to the migration DSL. e.g. index: true, index: :unique. Also adds multi-column indexes via index: { with: ['other', 'columns'] }. Also adds things like:if_exists` to drop_index, and more consistent behavior for duplicate. Plus, for postgresql it adds support for expressions, operator classes, and case-insensitive. [It's feeling a bit like a bit of a grab-bag. Maybe pull the postgresql enhancements into a separate gem?]

  • schema_pg_enums

    Adds support for postgresql enums. [Or name it just schema_enums. It'll only support posgresql for now, but who knows, maybe later...]

  • schema_views

    Adds support for create_view etc.

  • schema_foreign_keys

    [Not yet teased out from the schema_plus code, not yet cleaned up to use schema_monkey for all monkey patching] The foreign key DSL additions and behavior.

  • schema_plus

    This will just include all(?) the others and deprecate SchemaPlus.config in favor of SchemaForeignKeys.config


Thanks for any comments!

This was referenced Jan 17, 2015
@lowjoel
Copy link
Member

lowjoel commented Jan 17, 2015

I think schema_pg_enums can just be schema_enums, so you won't have nasty renaming problems later on when MySQL support is added (for example)... You can specify clearly in the Gemspec and Readme that it only works on Postgres for now, I think?

@ronen
Copy link
Member Author

ronen commented Jan 17, 2015

@lowjoel yeah will go with just schema_enums.

Re schema_plus_index i'm thinking now that it should be split up into separate gems:

  • one that adds features to AR for how you add and interact with indexes (shortcuts, if_exists, etc) but doesn't add capabilities to the indexes.
  • a separate one (or more?) that adds support for database index capabilities AR doesn't support (expressions, operator classes, etc).

Though I'm stumped as to what names to give them. schema_plus_index and schema_index_extras? Any better ideas?

@lowjoel
Copy link
Member

lowjoel commented Jan 17, 2015

I'm mixed on this one too.

I think for now, the initial grouping proposed at the top of the PR is already modular enough. I don't think there's significant benefit in breaking the schema_plus_index gem down further. Later on, we can always just give steps for migrating from the combined gem to the split gem, if there is significant code bloat...

I'm starting to be a little concerned about the things we do for Postgres though. Maybe that needs to be factored out? Or we expose high-level features, and implement adapters for each of the database servers?

@ronen
Copy link
Member Author

ronen commented Jan 18, 2015

@lowjoel yeah, could maybe factor out Postgres. But that's back to arguing for schema_pg_enum rather than schema_enums, no? I'm likewise unsure which way is best to go.

Re whether schema_plus_index should be broken up. These days I'm liking the idea of pushing Separation of Concerns/Single Responsibility as much as possible, making me lean towards erring on the side of too many tiny single-purpose gems. (I'm probably overreacting to having looked for too long at the bloated jumble of schema_plus 1.x : )

...though whether a "single-purpose" would be "enums" or "postgresql enums" is unclear...

@donv, @lowjoel, @dimonzozo, @methodmissing, @tovodeverett, @bacrossland (or anyone else!) -- any thoughts on all this?

BTW it's looking like there's one more gem that doesn't fit in with the original grouping:

  • schema_plus_column - adds methods to the Column object, including column.indexes and column.unique? (I know that's used by schema_validations)

@andreynering
Copy link

@ronen What about supporting PostgreSQL check constraints?

Take a look at this gem: https://github.com/take-five/postgresql-check. It is not much popular but seems to work fine.

Until now, I ever had to use structure.sql when I want to use it.

(I don't know if MySQL or SQLite support checks.)

@ronen
Copy link
Member Author

ronen commented Jan 20, 2015

@andreynering as far as I know MySQL does't and SQLite does. So, since more than one dbms already supports it, in terms of this refactoring discussion, that'd argue for a generic gem schema_plus_check gem rather than a pg-specific gem.

As to whether to actually go ahead and make one -- I'm hoping that once this refactor is done, the schema_monkey gem will make it relatively easy to create small gems like this. And it won't require any discussions of whether it's worth "supporting in schema_plus".

In this particular case, if https://github.com/take-five/postgresql-check already works, there might not be any particular reason to write another one. (Unless the that gem's monkey patching and schema_plus's monkey patching collide, which could happen.)

BTW there's a long-standing request for check constraints (#92), and @adymo started implementing it (#128) a while back. Unfortunately he ran into difficulty and asked for help which I realize now I never responded to (sigh, I suck -- sorry @adymo). Again I'm hoping that once I get all this installed, it'll be a lot easier to make a small gem to do it.

[For more discussion specific to check constraints, let's move it back to #92 or open a new issue; I'd like to keep this issue focused on general architectural issues of how to best organize the refactor.]

Thanks!

@ronen
Copy link
Member Author

ronen commented Jan 23, 2015

FYI my latest thought is that it'd be good to have a reasonable consistent naming scheme. Here's my proposal:

  1. Name all the schema_plus feature gems schema_plus_xxxx.

  2. Name db-specific gems schema_plus_<db>_xxxx.

  3. Don't sweat having long names if they're descriptive.

  4. schema_plus will be a wrapper around all schema_plus_* gems

    Based on the above, my current plan is to move towards:

    • schema_plus_columns
    • schema_plus_db_default
    • schema_plus_default_expr
    • schema_plus_enums
    • schema_plus_foreign_keys
    • schema_plus_indexes
    • schema_plus_pg_indexes
    • schema_plus_tables
    • schema_plus_views
    • Future gems might include schema_plus_check_constraints , etc.
  5. Utility/support gems will be named schema_yyyy (schema_dev, schmea_monkey)

  6. Other gems that go beyond simple API enhancements to adding new behaviors that not all people will want: Name them whatever makes sense for them, leaning towards schema_zzzz. E.g. the existing schema_validations and schema_associations gems.

@donv
Copy link
Contributor

donv commented Jan 23, 2015

Sounds good to me.

@lowjoel
Copy link
Member

lowjoel commented Jan 24, 2015

👍

@ronen
Copy link
Member Author

ronen commented Jan 26, 2015

Thanks for the feedback. FYI I've pushed out the first two feature gems:

I've also pushed out https://github.com/SchemaPlus/schema_monkey but it's still entirely undocumented, and doesn't even have specs :( Will need to address that soon. And there's plenty more to add to it, I'll be doing that incrementally as I peel out more feature gems from the refactor branch.

@ronen
Copy link
Member Author

ronen commented Feb 10, 2015

FYI, I've pushed out schema_plus 2.0.0.pre4. Some major refactoring and cleanup: schema_monkey has been split into four(!) gems.

  • modware - generic middleware stack library.
  • schema_monkey - convention-based insertion of modules into ActiveRecord and creation and use of middleware stacks.
  • schema_monkey_rails - for rails apps, initializes schema_monkey appropriately in a Railtie
  • schema_plus_core - a client of schema_monkey, it defines the core extension API, as a collection of middleware stacks

All are documented, all have specs with 100% coverage.

schema_plus itself still has a grabbag of the feature-gems-to-be; many of them do some monkey-patching that should move into schema_plus_core. But it's getting cleaner and easier to do.

@ronen
Copy link
Member Author

ronen commented Feb 11, 2015

FYI pushed out schema_plus 2.0.0.pre5, with support for views pulled out into a feature gem:

@donv
Copy link
Contributor

donv commented Feb 11, 2015

Great going, @ronen !

@ronen
Copy link
Member Author

ronen commented Feb 13, 2015

FYI pushed out schema_plus 2.0.0.pre6 and schema_plus_columns

@ronen
Copy link
Member Author

ronen commented Feb 13, 2015

(Oh it also includes the bug fix for #203 pulled over from the 1.x branch)

@ronen
Copy link
Member Author

ronen commented Feb 14, 2015

FYI pushed out schema_plus 2.0.0.pre7 and schema_plus_tables

@ronen
Copy link
Member Author

ronen commented Feb 27, 2015

pushed out schema_plus 2.0.0.pre8 -- no longer includes now-unneccessary schema_monkey_rails.

(also some other changes on the way towards pulling out auto-fk)

@ronen
Copy link
Member Author

ronen commented Mar 13, 2015

pushed out schema_plus 2.0.0.pre9 and schema_plus_enums

@ronen
Copy link
Member Author

ronen commented Mar 22, 2015

released schema_plus 2.0.0.pre10 which support AR 4.2.1 as well as 4.2.0

@ronen
Copy link
Member Author

ronen commented Mar 22, 2015

released schema_plus 2.0.0.pre11 and schema_plus_db_default

@ronen
Copy link
Member Author

ronen commented Apr 1, 2015

released schema_plus 2.0.0.pre12 and schema_plus_default_expr

@ronen
Copy link
Member Author

ronen commented May 17, 2015

releases 2.0.0.pre13, .pre14, and .pre15 were bug fixes.

now released schema_plus 2.0.0.pre16 and schema_plus_foreign_keys

@ronen
Copy link
Member Author

ronen commented May 22, 2015

extracted schema_auto_foreign_keys...

...and released version 2.0.0

finally. whew.

@ronen ronen closed this as completed May 22, 2015
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants