Can't load function based holidays with .load_custom about holidays

[md5]

@ptrimble if your refactored code is pushed to a branch somewhere, I'd be happy to help test. I'm running into this problem as well.

[ppeble]

Hm, that's interesting. The original intent I had was to simply reuse the existing definition merging logic but expose it so it could be called by load_custom. Clearly whatever I intended isn't happening in all cases.

Would you be able to put up a gist (or copy/paste) the custom definition? That way I can write some tests that explicitly hit this scenario and ensure we never encounter it again.

Thanks for the report! I'll look into this in the meantime.

[ppeble]

Also, just want to point out that with the refactor some of these functions have moved.

[ppeble]

I also think I realized a bug in my load_custom logic. I don't think we are loading and storing the custom procs anywhere. What you are doing is attempting to access an existing proc that exists in other places. But what if you want to add a new custom proc? I don't think it will work!

[ppeble]

Man, this bug is subtle as hell because of the weird require stuff we do on loading of regions. I'm gonna need some time to figure out a good solution. 😦

EDIT: My gut is telling me that I need to pass the custom regions into the merge_defs method and store them as something separate that can be searched rather than just expecting the require to pull them in. That's the issue here.

We need to move away from using require to get all of the def info.

[ppeble]

Bwuh. I don't see any other way out of this other than a breaking change and updating the way that we handle custom procedures. I have a pretty clear idea on how to handle it but it'll require me to change every def that has custom procs. I think this is the right long-term play but it'll take me a bit of time...

[elbartostrikesagain]

I had the same problem. Also, it seems for some cases function is a string passed containing (year) and some cases it only had the function name. I got fed up and didn't bother to figure out why, leading to this horrid code below haha. I also had problems defining custom functions in yaml like the US file(day_after_thanksgiving) so I defined them outside the yaml. But maybe someone else will find this useful...

module Holidays::Definition::Repository
  class ProcCache
    def lookup(function, year)
      proc_key = Digest::MD5.hexdigest("#{function.to_s}_#{year.to_s}")
      if function.is_a?(String)
        function = "Holidays." + function
        function += "(year)" unless function.include?("(year)")
        f = Proc.new do |year|
          eval("#{function}")
        end
      else
        f = function
      end
      @proc_cache[proc_key] = f.call(year) unless @proc_cache[proc_key]
      @proc_cache[proc_key]
    end
  end
end

[ppeble]

@elbartostrikesagain Thank you! Yes, it is helpful, and I am very close to having a PR to address this. The holidays took a lot out of me so I didn't make as much progress during January as I wanted but I should have something that addresses this shortly.

Thanks again for providing this snippet.

[ppeble]

Another small update: I have done a LOT of refactoring and am nearly there. I am having some issues only with the JP holidays and how many nested custom methods calls are there. I am confident I can solve this soonish.

[ppeble]

@md5 Thanks, I am 99.5% done. The snag is that the :jp region has what amounts to 'nested' custom method calls, requiring me to do some late refactoring to make it all work. That's why there was a delay. This is the only region to do that. :(

I believe I've solved it and I am hoping to get a final PR out shortly. That should unblock a lot of this. Sorry!

[ppeble]

@md5 @ttwo32

Oof! Finally got to green. Turns out I introduced a regression that was caught by the tests for various regions but it took fooooorrreeevvver for me to find it. Should be good now, I think. PR is up.

I'm still going to be putting this through the various tests so I would appreciate any help with testing or comments!

[ppeble]

FYI, current testing shows that I have not totally licked this issue but I feel I am really close. For the moment please stick to comments on the code itself rather than testing until I give the green light.

[ppeble]

Okay! All changes pushed up. I have included how I tested it. At this stage I am in the cleanup/benchmarking/testing phase. I believe I have solved the problem root problem as stated by @angelim.

I will clean up the outstanding build issue, post some benchmarking numbers so everyone is clear how things are changing performance-wise, and then spend time merging the existing PRs and managing merge conflicts on my PR. This will probably take a few days.

If no one (looking at @md5 and @ttwo32, specifically, since you volunteered) have no objections after that then I will plan for a merge and major version bump.

[ppeble]

Alright, final update before closing this issue: this has been fixed by the PR I mentioned above. I will be releasing version 4.0.0 sometime in the next few days, at which point I will close this issue. Based on my testing it is now fixed.

If anyone sees evidence that it is not fixed then please let me know here and I will reopen to investigate. Thanks!

[knut2]

Should Holidays.load_custom support also easter?
If yes, then please check my version https://github.com/knut2/holidays/tree/custom_easter. I extended the unit tests with a test for custom easter holidays.
The test fails with NoMethodError: undefined method 'call' for "easter(year)+1":String

If this is a bigger problem, I would recommend to make only a remark in the readme _(Holidays.load_custom _don't support easter logic)__. The problem could be solved later.

[ppeble]

Hello @knut2! Yes, they should work. Your unit tests are great! I am going to steal it and commit it on my branch in your name if that is okay. 😄

To answer your question: yes, it should work, but the issue here is that I changed the definition format a little bit. The easter(year)+1 function name will no longer work. The name of the function (plus the arguments) is what internally is used as the function ID when we look up the proc from the in-memory repository. So to reference it you would do easter(year). To modify the output I added a new function_modifier key/value pair in the definitions. You should set it to 1. The refactored code will calculate the function and then apply the modifier to the result.

Here is an example in my tests: https://github.com/ptrimble/holidays/blob/issue-144/test/data/test_single_custom_holiday_with_custom_procs.yaml#L5-L6

This example is a defined method but the same idea works for easter or any of the other 'common' methods.

I updated the README for the definitions to fully spell this out.

Few final notes:

• splitting the +1 modifiers out of the function name was required because we are no longer simply assuming that the function key in the YAML files is ruby code to execute. It is a name that is looked up. This makes the YAML files a bit less Ruby-centric. My ultimate goal is to make totally agnostic definitions that can be loaded into other languages but that is a long way off.
• I am still looking for a way to organize the available 'common' methods, like easter or orthodox_easter. Right now a contributor just kinda has to know them and that's not ideal.
• I am going to update the CHANGELOG to make it clear that any custom definitions must change the easter() references or they will see failures. Thanks for the notice!

Please try changing your custom definition to the format I highlighted above and let me know if it still doesn't work. If it's still failing could you please make a gist of your custom definition so I could test it myself?

Thanks!

[ppeble]

Just to make it clear, I wrote up some custom defs that used easter. Here:

months:
  0:
  - name: Custom Good Friday
    regions: [custom_single_file]
    function: easter(year)
    function_modifier: -2
  6:
  - name: Company Founding
    regions: [custom_single_file]
    mday: 20

Then I loaded up the gem and tested it:

% be rake console
irb -rubygems -I lib -r holidays.rb
irb(main):001:0> Holidays.load_custom('test.yaml')
=> {"easter(year)"=>#<Proc:0x007f51058af1a8 (lambda)>, "orthodox_easter(year)"=>#<Proc:0x007f51058ada10 (lambda)>, "orthodox_easter_julian(year)"=>#<Proc:0x007f51058a7ef8 (lambda)>, "to_monday_if_sunday(date)"=>#<Proc:0x007f51058a7e08 (lambda)>, "to_monday_if_weekend(date)"=>#<Proc:0x007f51058a7c50 (lambda)>, "to_weekday_if_boxing_weekend(date)"=>#<Proc:0x007f51058a7a70 (lambda)>, "to_weekday_if_boxing_weekend_from_year(year)"=>#<Proc:0x007f51058a7958 (lambda)>, "to_weekday_if_weekend(date)"=>#<Proc:0x007f51058a7818 (lambda)>, "calculate_day_of_month(year, month, day, wday)"=>#<Proc:0x007f51058a7598 (lambda)>, "to_weekday_if_boxing_weekend_from_year_or_to_tuesday_if_monday(year)"=>#<Proc:0x007f51058a7408 (lambda)>, "xmas_to_weekday_if_weekend(year)"=>#<Proc:0x007f51058a7188 (lambda)>, "to_tuesday_if_sunday_or_monday_if_saturday(date)"=>#<Proc:0x007f51058a6eb8 (lambda)>}
irb(main):002:0> Holidays.on(Date.civil(2016, 3, 27), :custom_single_file)
=> []
irb(main):003:0> Holidays.on(Date.civil(2016, 3, 25), :custom_single_file)
=> [{:date=>#<Date: 2016-03-25 ((2457473j,0s,0n),+0s,2299161j)>, :name=>"Custom Good Friday", :regions=>[:custom_single_file]}]
irb(main):004:0> Holidays.on(Date.civil(2016, 6, 20), :custom_single_file)
=> [{:date=>#<Date: 2016-06-20 ((2457560j,0s,0n),+0s,2299161j)>, :name=>"Company Founding", :regions=>[:custom_single_file]}]

I made a 'custom' Good Friday which is two days before easter and used the easter(year) method. Hopefully this is what you were looking for.

FYI, this specific thing was not working before the refactor so it was good for you to ask.

[ppeble]

Okay! Merged and released to rubygems.org. I'm gonna close this out, if anyone spots similar issues to this please comment here and I'll reopen. Thanks for all the feedback during this!