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.

Sign up for GitHub

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

Jump to bottom

fix dumping expression indexes #27

Merged
merged 2 commits into from Dec 31, 2018
Merged

fix dumping expression indexes #27

merged 2 commits into from Dec 31, 2018

Conversation

urkle
Copy link
Member

@urkle urkle commented Dec 21, 2018

Fixes scenario where the dumper fails with an expression index exists for multiple columns.

t.index "date(created_at), id", name: "index_expression", where: "status = 0"

@urkle urkle self-assigned this Dec 21, 2018
@urkle urkle added this to In progress in Rails 5.2 Support via automation Dec 21, 2018
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.3%) to 99.454% when pulling 6819665 on fix-index-expression-dumping into 7974a67 on master.

4 similar comments
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.3%) to 99.454% when pulling 6819665 on fix-index-expression-dumping into 7974a67 on master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.3%) to 99.454% when pulling 6819665 on fix-index-expression-dumping into 7974a67 on master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.3%) to 99.454% when pulling 6819665 on fix-index-expression-dumping into 7974a67 on master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.3%) to 99.454% when pulling 6819665 on fix-index-expression-dumping into 7974a67 on master.

@coveralls
Copy link

coveralls commented Dec 21, 2018 •
edited

Coverage Status

Coverage increased (+0.2%) to 100.0% when pulling c1ec84d on fix-index-expression-dumping into 7974a67 on master.

lowjoel
lowjoel reviewed Dec 22, 2018
View reviewed changes
Copy link
Member

@lowjoel lowjoel left a comment

Choose a reason for hiding this comment

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

Sorry, this is all new to me. Can you explain what your changes do? The large regexes don't make sense to me 😓

spec/dumper_spec.rb Outdated Show resolved Hide resolved
lib/schema_plus/core/active_record/schema_dumper.rb
@@ -68,17 +68,36 @@ def table(table, _)
env.table.trailer = m[:trailer].split("\n").map(&:strip).reject{|s| s.blank?}
table_objects = m[:columns].strip.split("\n").map { |col|
cs = col.strip
result = nil
Copy link
Member

Choose a reason for hiding this comment

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

At this point I think it's worth splitting up to different functions for each arity. This seems rather convoluted...

Copy link
Member Author

Choose a reason for hiding this comment

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

it's all about priority of matching here.. as I have the expression index case, the generic case, and the column case.

Copy link
Member Author

Choose a reason for hiding this comment

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

once thought I could do, is extract it out into a CONST array of regex/lambda and reduce this block to just iterating those.. Then it'll be a little cleaner? e.g.

COLUMN_MATCHERS = [
 [%r{...}, ->(m) {}],
 [%r{...}, ->(m) {}],
].freeze

COLUMN_MATCHERS.find do |r, l| 
    m = cs.match(r)
   l.call(m) unless m.nil?
end

Copy link
Member

Choose a reason for hiding this comment

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

Yeah that would be awesome, thanks @urkle!

Copy link
Member Author

Choose a reason for hiding this comment

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

@lowjoel take a look now..

lib/schema_plus/core/active_record/schema_dumper.rb Outdated Show resolved Hide resolved
@urkle urkle force-pushed the fix-index-expression-dumping branch from 6819665 to 261efaf Compare December 26, 2018 03:46
@urkle urkle requested a review from lowjoel December 27, 2018 17:21
lowjoel
lowjoel reviewed Dec 27, 2018
View reviewed changes
lib/schema_plus/core/active_record/schema_dumper.rb
SchemaDump::Table::Index.new name: m[:name], columns: index_cols, options: eval("{#{m[:options]}}")
end
result = nil
TABLE_COLUMN_MATCHES.find do |(r, l)|
Copy link
Member

Choose a reason for hiding this comment

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

I think

result = nil
TABILE_COLUMN_MATCHES.find do |(r, l)|
  m = cs.match(r)
  result = ...
end
result

is equivalent to

TABLE_COLUMN_MATCHES.find do |(r, l)|
  m = cs.match(r)
  !m.nil? && l.call(m)  # or l.call(m) unless m.nil?
end

Copy link
Member Author

Choose a reason for hiding this comment

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

it's actually not.. as "find" returns the entry.. not the result of the block. (as I had that first and all tests failed)

Copy link
Member

Choose a reason for hiding this comment

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

Oops. I'll accept this.

This seems non-obvious though 🤔

@urkle urkle force-pushed the fix-index-expression-dumping branch from 5ca686a to 773c810 Compare December 28, 2018 16:31
@urkle urkle force-pushed the fix-index-expression-dumping branch from 773c810 to c1ec84d Compare December 30, 2018 21:45
@urkle urkle merged commit 0401dd1 into master Dec 31, 2018
Rails 5.2 Support automation moved this from In progress to Done Dec 31, 2018
@urkle urkle deleted the fix-index-expression-dumping branch December 31, 2018 14:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lowjoel lowjoel lowjoel approved these changes

Assignees

@urkle urkle

Labels
None yet
Projects
Rails 5.2 Support
  
Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@urkle @coveralls @lowjoel