thbar / kiba

Data processing & ETL framework for Ruby
https://www.kiba-etl.org
Other
1.75k stars 87 forks source link

Fixes issue with accessing methods defined outside of Kiba.parse block. #66

Closed ResumeNothing closed 5 years ago

ResumeNothing commented 5 years ago

This article explains the issue I was running into, along with the general fix. It's a side-effect of using instance_eval to create a DSL, which fortunately has a work around.

thbar commented 5 years ago

@ResumeNothing thanks for your PR! (and also for the one on kiba-common, I will get back to you shortly there!).

I will have to review this and also verify the impact on the multiple codebases I manage currently, to verify this indeed doesn't introduce side-effects or regressions.

Also, please note (see #65) that I will (in not too long) introduce a "contributor agreement", which will have to be accepted by each contributor to make sure the codebase is clear from an IP point of view & secure the use for everyone too.

All in all: thanks for your contributions, & bear with me until I can get back to you on both points!

ResumeNothing commented 5 years ago

@thbar You're welcome! No rush on the reviews, and let me know if there's anything you want me to change.

For the this one, the Truffle Ruby error appears to be unrelated to the code change. I tried updating to the latest version to see if that would fix it, but something still appears to be misconfigured.

ResumeNothing commented 5 years ago

I ended up running into another issue, even with this fix, so I decided to go in a different direction. You can feel free to close this if you don't find it otherwise useful or if it breaks on any of your other codebases.

This generates a failure with or without the fix, though the failure message is different:

  def some_method_that_references_kiba
    transform { |row| row }
  end

  def test_block_parsing_with_reference_to_outside_that_references_kiba
    control = Kiba.parse do
      source DummyClass
      # this raises an error
      some_method_that_references_kiba
    end
  end

Error message with the fix:

NoMethodError: undefined method `transform' for #<TestParser:0x00007fd75314e470>

Error message without the fix:

NameError: undefined local variable or method `some_method_that_references_kiba' for #<Kiba::Context:0x00007ff6df02bdd0>

Since a transform can be defined using a class instead of a method, I decided to just go with that instead.

thbar commented 5 years ago

@ResumeNothing thanks for the update!

A pattern may help you out here (this is used in the Kiba testing suite & also in a lot of my codebases these days, but I realise it's not in the documentation yet - will fix that):

module DSLExtensions
  module MyExtension
    def meta_transform
      transform { |row| row }
      transform { |row| row }
      transform { |row| row }
    end
  end
end

Then in your job declaration:

require 'my_extension'

Kiba.parse do
  extend DSLExtensions::MyExtension

  meta_transform
end  

This can help develop little keywords, that you can include in the DSL selectively.

(of course it's still recommended that you extract transforms to classes, especially if you manage to make them very generic & reusable).

You can see 2 examples of that in kiba-common here:

https://github.com/thbar/kiba-common/tree/master/lib/kiba-common/dsl_extensions

Hope this helps - and I'll make sure to better advertise this in the near future.

thbar commented 5 years ago

@ResumeNothing I've fully documented how to do this in the Kiba wiki here:

https://github.com/thbar/kiba/wiki/How-to-extend-the-Kiba-DSL%3F

I'll close the PR for now. Let me know if you have further questions though, or if this is "good enough"!

ResumeNothing commented 5 years ago

@thbar Thanks for adding those to the documentation! I'll be sure to give that a try since it's a closer match to our legacy code; I have a lot less to update if I don't have to convert everything to classes right away :)

thbar commented 5 years ago

@ResumeNothing well ultimately I find that on large codebases I use both DSLExtensions and classes, for different topics. I will blog about that at some point!

Good luck!