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

extend result objects with additional methods #7

Closed
wants to merge 3 commits into from

Conversation

ermolaev
Copy link
Contributor

Often need objects with additional behavior

maybe it’s better to use other name for the parameter - instance_methods or ...

What do you think?

@SamSaffron
Copy link
Member

This is a bit rough here on a few counts,

  1. It introduces extra work every time query is called to decide what to do with the materializer

  2. It has some potential for poisoning the materializer cache

I can get the desire here to have new_row_matrializer return a custom materializer, but instead for now I would recommend you simply override that method.

A more complete interface here to support this imo would be:

DB.query_typed(User, sql, *args)

At least that way we can very strategically create this so it has zero impact on existing query. Then User is simply used instead of going to the materializer cache.

There are tons of details in getting this right though, I would be worried about introducing too much surface area here.

@ermolaev
Copy link
Contributor Author

It has some potential for poisoning the materializer cache

I'm sorry, but I do not see

At least that way we can very strategically create this so it has zero impact on existing query. Then User is simply used instead of going to the materializer cache.

if I understand you correctly

class User
  attr_accessor :name, email

  def last_name
    name.split(:name).last
  end 
end

DB.query_typed(User, 'select name, email where id = ?', 10)

this is inconvenient because it forces you to create different classes for different select columns, and monitor the changes in the columns in the query

@ermolaev ermolaev mentioned this pull request Jan 30, 2020
@ermolaev
Copy link
Contributor Author

reworked in #13

@ermolaev ermolaev closed this Jan 30, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants