search

dpickett / has_barcode

Nice class method wrapper for barby
MIT License
51 stars 18 forks source link

Allow for type class override, and provide access to the config from the receiving model. #7

Closed kevinelliott closed 11 years ago

kevinelliott commented 11 years ago

Certain types will not load with has_barcode, such as :ean_13 which looks for Bardy::Ean13 instead of the correct Bardy::EAN13. With this pull request, you can pass has_barcode the :type_class with the full class and it will work correctly.

Additionally, I have exposed the config so that the original type or other options are available from the model (in case you want to validate your barcode and output the type in the validation error).

dpickett commented 11 years ago

this is great, thanks. Can we make it so it introspects on the opt[:type] instead of specifying a new option?

kevinelliott commented 11 years ago

Having seen your response to another code submission about this topic, I wanted to do that. The problem is that your existing code uses opt[:type] to convert the symbol to a string and pulls in the require for a file in barby that matches the string.

There are two problems with introspecting on opt[:type] for a Class and manipulating the class name:

1) The class itself will not exist in the rails app's model's namespace until has_barcode requires the corresponding file containing the class. Thus, you can't pass an actual Class into the opts[:type], and must use a string containing the class name.
2) If you were to check for String type and manipulate the contents, you would have to chop off any namespace, but you still might be left with a class that does not match the filename necessary to kick off the correct require statement.

Additionally, I believe that class_name is reasonable, because this is how the other Rails relationship methods work when you wish to do overrides (i.e. has_many :sheep, class_name: 'Animal'). Considering that has_barcode is essentially implementing a Rails-style polymorphic relationship to Model, it would indicate that it should follow Rails standards.

I'm happy to fix my submission to do what you desire, so let me know how to proceed.

On Jan 8, 2013, at 2:14 PM, Dan Pickett notifications@github.com wrote:

this is great, thanks. Can we make it so it introspects on the opt[:type] instead of specifying a new option?

— Reply to this email directly or view it on GitHub.

kevinelliott commented 11 years ago

Thoughts?

dpickett commented 11 years ago

I still think there's a way to provide this functionality without adding an additional option. We could check the symbolized name against a whitelist of barby support formats as a means to support legacy calls, and otherwise constantize based on a fully qualified string. I think that would be my preference.

I don't think it's unreasonable to require the developer to issue a require statement for the format they want.

kevinelliott commented 11 years ago

It's strange that we wouldn't want to stay consistent with Rails has_many, but we can do what you want when I have more time.

On Jan 11, 2013, at 5:50 AM, Dan Pickett notifications@github.com wrote:

I still think there's a way to provide this functionality without adding an additional option. We could check the symbolized name against a whitelist of barby support formats as a means to support legacy calls, and otherwise constantize based on a fully qualified string. I think that would be my preference.

I don't think it's unreasonable to require the developer to issue a require statement for the format they want.

— Reply to this email directly or view it on GitHub.