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

Extract iterator method in AR::SchemaDumper #16297

Merged
merged 1 commit into from Jul 28, 2014

Conversation

calebhearth
Copy link
Contributor

Gems which wish to tie into ActiveRecord::SchemaDumper need to duplicate this
logic currently. Foreigner is one such example, as is a library I'm
currently working on but which hasn't been released yet:

def tables_with_foreign_keys(stream)
  tables_without_foreign_keys(stream)
  @connection.tables.sort.each do |table|
    next if ['schema_migrations', ignore_tables].flatten.any? do |ignored|
      case ignored
      when String; table == ignored
      when Regexp; table =~ ignored
      else
        raise StandardError, 'ActiveRecord::SchemaDumper.ignore_tables accepts an array of String and / or Regexp values.'
      end
    end
    foreign_keys(table, stream)
  end
end

Extract the skip logic to a method, making it much simpler to follow this same
behavior in gems that are tying into the migration flow and let them dump only
tables that aren't skipped without copying this block of code. The above code
could then be simplified to:

    def tables_with_foreign_keys(stream)
      tables_without_foreign_keys(stream)
      @connection.tables.sort.each do |table|
        foreign_keys(table, stream) unless ignored?(table)
      end
    end

This could be further simplified to assume tables, but for my use case I'm
trying to add views which wouldn't be returned by the tables method.

@calebhearth
Copy link
Contributor Author

@schneems @derekprior

@calebhearth
Copy link
Contributor Author

Since I now realize it may not be clear, the functionality I'm moving around is what lets ActiveRecord::SchemaDumper.ignore_tables skip specific tables when dumping to db/schema.rb.

https://github.com/calebthompson/rails/blob/extract-iterator-method/activerecord/lib/active_record/schema_dumper.rb#L14-L16

Gems which wish to tie into ActiveRecord::SchemaDumper need to
duplicate this logic currently. [Foreigner] is one such example, as is a
library I'm currently working on but which hasn't been released yet:

    def tables_with_foreign_keys(stream)
      tables_without_foreign_keys(stream)
      @connection.tables.sort.each do |table|
        next if ['schema_migrations', ignore_tables].flatten.any? do |ignored|
          case ignored
          when String; table == ignored
          when Regexp; table =~ ignored
          else
            raise StandardError, 'ActiveRecord::SchemaDumper.ignore_tables accepts an array of String and / or Regexp values.'
          end
        end
        foreign_keys(table, stream)
      end
    end

[Foreigner]: https://github.com/matthuhiggins/foreigner/blob/master/lib/foreigner/schema_dumper.rb#L36-L43

Extract the skip logic to a method, making it much simpler to follow
this same behavior in gems that are tying into the migration flow and
let them dump only tables that aren't skipped without copying this block
of code. The above code could then be simplified to:

    def tables_with_foreign_keys(stream)
      tables_without_foreign_keys(stream)
      @connection.tables.sort.each do |table|
        foreign_keys(table, stream) unless ignored?(table)
      end
    end

It also, in my opinion, simplifies the logic on ActiveRecord's side, and
clarifies the intent of the skip logic.
@schneems
Copy link
Member

👍 the change is relatively trivial, and the resulting method extraction is cleaner. If it provides a useful interface in the process i'm for it.

rafaelfranca added a commit that referenced this pull request Jul 28, 2014
Extract iterator method in AR::SchemaDumper
@rafaelfranca rafaelfranca merged commit 64e2e81 into rails:master Jul 28, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants