search

fastruby / harvesting

Ruby wrapper for the Harvest API v2
MIT License
28 stars 30 forks source link

fix incompatibility with ruby 3 #64

Closed jhendley25 closed 3 years ago

jhendley25 commented 3 years ago

This hasn't been heavily tested at all, but ran into this and figured I'd open a PR. Simply passing the block fixes the errors I was seeing.

jhendley25 commented 3 years ago

Sure. The initial error I ran into after upgrading to Ruby 3 was: tried to create Proc object without a block which I tracked down to that line in the gem.

After digging more into it, I think the ruby upgrade may be a red herring - our harvest account may have just never had enough projects to require pagination until now. I confirmed that line fails on ruby 2.6 as well by just doing this in irb:

def getting_block
  yield if block_given?
end

getting_block(&Proc.new)

which throws the same error. Or there may be another cause... sorry if this is less than helpful.

jhendley25 commented 3 years ago

Actually it looks like the Ruby 3 upgrade had nothing to do with the bug. I don't know exactly why it was breaking for me but passing the test suite, but that last commit explicitly accepts and forwards the block to the recursive each, which fixes everything for me. Let me know if I can help here or if I should just close the PR.

timirwin commented 3 years ago

@jhendley25 Did you have a different work around? I have the same issue but it works with your change. My sample code iterates over users where I have more than 100 users.

client = Harvesting::Client.new
client.users.each do; end

Before your branch, I get ArgumentError: tried to create Proc object without a block but no error after.

jhendley25 commented 3 years ago

@timirwin I'm just using my branch until this PR gets merged or another fix is in place.

etagwerker commented 3 years ago

@jhendley25 Could you review the failure in CI? It seems relevant to the change you are submitting.

jhendley25 commented 3 years ago

@etagwerker Looks like CI is passing now. The previous issue was because my initial fix broke pagination. It's working properly now.

etagwerker commented 3 years ago

@jhendley25 @timirwin Just released this fix in v0.5.1: https://rubygems.org/gems/harvesting/versions/0.5.1 -- Thanks! 💯