custom helper preprocesor

[lporras]

Hey guys,

I have a suggestion about using helpers on the importer, for instance it is very common to add a helper to normalize the data received when reading and excel like this:

class ProductImporter < ActiveImporter::Base
  imports Product

  column 'Category', :category do |value|
    normalize(value)
  end

  private

  def normalize(value)
     value.strip.downcase
  end
end

but if we have a lot of column on Product we have to define a lot of similar blocks. It would be nice to have something like this:

class ProductImporter < ActiveImporter::Base
  imports Product

  column 'Category', :category, :normalize
  column 'Section', :section, :normalize

  ...  

  private

  def normalize(value)
     value.strip.downcase
  end
end

So in this case the normalize helper would be used to set each column of the product.
I know we could use also on :row_processing to call the normalize method inside the on row_processing block but I love the idea of having an alternative way ActiveImporter DSL.

what do you think? @gnapse ?

Best,

Luis Porras

[gnapse]

Although this seemed nice at first, on a second thought I think this is just syntactic sugar. And while that alone is not bad (syntactic sugar can be good), I think in this case there's little benefit in return. You could also write something like this:

class ProductImporter < ActiveImporter::Base
  imports Product

  column('Category', :category) { |v| normalize(v) }
  column('Section', :section) { |v| normalize(v) }

  ...  

  private

  def normalize(value)
     value.strip.downcase
  end
end

We could even make the following notation work too, if it's not already:

  column 'Category', :category, -> (v) { normalize(v) }
  column 'Section', :section, -> (v) { normalize(v) }

I'd even say this is more clear to a new user coming to the library. Your proposal would need people read that feature's documentation, or maybe infer what's happening from the names of things, and trust that what they're inferring is correct.

Would you consider this an acceptable alternative @lporras?

[gnapse]

BTW, I should mention that if what you're doing in some of these helpers is merely invoking a method to the actual value, I suspect this notation is also available:

  column 'Category', :category, &:downcase

That would be equivalent to:

  column('Category', :category) { |v| v.downcase }

It's not applicable to your case above because normalize invokes two methods in sequence to the value of the column, but it might be worth knowing for some cases where you wouldn't even need to declare the helper function to begin with.

[lporras]

@gnapse, thanks for your quick response, I'm going to test using this:

&:downcase

notation, and if it works then I think i could add the normalize method to the String class and leave it like this:

column 'Category', :category, &:normalize

and about the question related to syntactic sugar, I really like this way you proposed:

column 'Category', :category, -> (v) { normalize(v) }
column 'Section', :section, -> (v) { normalize(v) }

[gnapse]

@lporras I would strongly advise against this:

add the normalize method to the String

Monkey-patching is generally being avoided now more than before. It sounds like a good idea, but you can end up polluting the namespace of a class just for too little in return. I stress the idea that the gain in a slightly cleaner and shorter syntax is not enough reason to do it.

At the very least I'd urge you to use the newer Ruby language feature called refinements. It allows you to limit a monkey-patching to just inside the scope inside a specific module. Code outside that module is not affected.