Grow from Small Simple Steps

Transcript

1. Grow from Small Simple Steps
   Jack Chen
   @aquajach
   

   View Slide

2. Grow from S3
   Jack Chen
   @aquajach
   

   View Slide

3. THE PROBLEM
   gem 'activerecord-import'
   books = []
   10.times do |i|
   books << Book.new(:name => "book #{i}")
   end
   Book.import books
   

   View Slide

4. THE PROBLEM
   class Book < ActiveRecord::Base
   enum status: [:drafted, :finished, :published]
   #book.status = 0 or book.status = 'drafted'
   end
   create_table 'books' do |t|
   t.integer 'status', default: 0
   end
   books = []
   10.times do |i|
   books << Book.new(:name => "book #{i}")
   end
   Book.import books #=> ArgumentError: '0' is not a valid status
   

   View Slide

5. WHY IT FAILS
   # Returns the list of a table's column names, data types, and default values.
   def column_definitions(table_name) # :nodoc:
   exec_query(<<-end_sql, 'SCHEMA').rows
   SELECT a.attname, format_type(a.atttypid, a.atttypmod),
   pg_get_expr(d.adbin, d.adrelid), a.attnotnull, a.atttypid, a.atttypmod
   FROM pg_attribute a LEFT JOIN pg_attrdef d
   ON a.attrelid = d.adrelid AND a.attnum = d.adnum
   WHERE a.attrelid = '#{quote_table_name(table_name)}'::regclass
   AND a.attnum > 0 AND NOT a.attisdropped
   ORDER BY a.attnum
   end_sql
   end
   

   View Slide

6. WHY IT FAILS
   # Returns the list of a table's column names, data types, and default values.
   def column_definitions(table_name) # :nodoc:
   exec_query(<<-end_sql, 'SCHEMA').rows
   SELECT a.attname, format_type(a.atttypid, a.atttypmod),
   pg_get_expr(d.adbin, d.adrelid), a.attnotnull, a.atttypid, a.atttypmod
   FROM pg_attribute a LEFT JOIN pg_attrdef d
   ON a.attrelid = d.adrelid AND a.attnum = d.adnum
   WHERE a.attrelid = '#{quote_table_name(table_name)}'::regclass
   AND a.attnum > 0 AND NOT a.attisdropped
   ORDER BY a.attnum
   end_sql
   end
   text
   

   View Slide

7. WHY IT FAILS
   class ActiveRecord::Base
   class << self
   def import
   ...
   array_of_attributes = models.map do |model|
   column_names.map do |name|
   model.read_attribute_before_type_cast(name.to_s)
   end
   end
   ...
   instance = new do |model|
   hash.each_pair{ |k, v| model.send("#{k}=", v) } #=> ArgumentError: '0' is not a valid status
   end
   ...
   end
   end
   end
   

   View Slide

8. TEST?
   class Book < ActiveRecord::Base
   enum status: [:draft, :published] if ENV['AR_VERSION'].to_i >= 4.1
   #AR_VERSION = 3.1, 3.2, 4.0, 4.1, 4.2
   end
   

   View Slide

9. TEST?
   class Book < ActiveRecord::Base
   enum status: [:draft, :published] if ENV['AR_VERSION'].to_i >= 4.1
   #AR_VERSION = 3.1, 3.2, 4.0, 4.1, 4.2
   end
   
   

   View Slide

10. View Slide

11. View Slide

12. It’s the best PR I have ever seen
    

    View Slide

13. THE REALITY
    
    Only for PostgreSQL? Is Rails’ enum not smart enough?
    

    View Slide

14. CRACK ON
    
    PR: read_attribute_before_type_cast => read_attribute
    Maintainer: why read_attribute_before_type_cast is used?
    

    View Slide

15. Rails 1.2
    Maintainer: It was back to 2007. Use read_attribute if it gets test case pass
    Maintainer: why read_attribute_before_type_cast is used?
    

    View Slide

16. 
    Maintainer: Another method handles casting. Using read_attribute might be ok
    Maintainer: It was back to 2007. Use read_attribute if it gets test case pass
    

    View Slide

17. 
    Me: Just benchmark it, read_attribute is 50% slower for casting
    Maintainer: Another method handles casting. Using read_attribute might be ok
    

    View Slide

18. Committer: Simple fix
    instance = new do |model|
    # hash.each_pair{ |k, v| model.send("#{k}=", v) }
    hash.each_pair{ |k,v| model[k] = v }
    end
    
    Me: Just benchmark it, read_attribute is 50% slower for casting
    

    View Slide

19. Committer: Simple fix
    instance = new do |model|
    # hash.each_pair{ |k, v| model.send("#{k}=", v) }
    hash.each_pair{ |k,v| model[k] = v }
    end
    ☹
    Me: Seems working, but…
    

    View Slide

20. 
    Maintainer: I’m going to merge this and create another PR
    Me: Seems working, but…
    

    View Slide

21. ☹
    

    View Slide

22. MY TAKEAWAYS
    • ActiveRecord::ConnectionAdapters
    • ActiveRecord different assignment methods and type casting
    • Default value in PostgreSQL
    • Libraries/gems supporting multiple ruby version and database adaptors
    • Rails enum
    

    View Slide

23. Don’t believe in Stars
    

    View Slide

24. Don’t think of making a perfect PR at the beginning
    

    View Slide

25. View Slide

26. Don’t compromise
    

    View Slide