Make ticker not trigger on NYSE holidays

[ForeverEndeavor]

This is a small addition to the ticker plugin to prevent the stock ticker from triggering during New York Stock Exchange trading holidays.

Adds an additional requirement of holidays to the plugin.

[johnmaguire]

Hi @ForeverEndeavor - thanks for the PR! This has definitely caused annoyances before.

One thing I'm a bit unsure about - it looks like the PR reimplements the logic from the required python-holidays repository. I think you've done this in order to scope it down to specific holidays? However, this means that if the logic changes upstream, we won't reap the benefits here unless we copy and paste the logic back over.

Did you consider an approach instead where you call get() with the current date and then check the return against expect strings? https://github.com/dr-prodigy/python-holidays#api

[ForeverEndeavor]

Thanks for looking at this @johnmaguire!

It's a little more complicated than calling get() because the python-holidays library only generates country/state/province-specific holiday sets, and NYSE doesn't really fit into any of those categories. The closest we could get using what is already provided in the library is instantiating an instance of US Holidays and then use get() with the current date with that instance, but the logic there gets a little muddy:

• The US observes federal holidays that aren't trading holidays (e.g., Juneteenth, although this may change starting in 2022)
• NYSE observes trading holidays that aren't base federal holidays (e.g., Good Friday)

In more complex cases like this one, the library recommends inheriting HolidayBase and populating holidays as your use-case requires (https://github.com/dr-prodigy/python-holidays#example-usage). Of course, doing it this way means we would need to augment our NYSEHolidays class in the event the exchange adds a new trading holiday.

Hopefully I didn't misunderstand the suggestion!

[johnmaguire]

Heya @ForeverEndeavor - that makes sense to me, thanks! I was missing the fact that NYSE observes holidays that aren't already encoded in the third-party package.

I would definitely like to merge this, but I do have a couple more notes:

1. Please add yourself to the CONTRIBUTORS file if you wish! :)
2. Would you mind re-ordering the imports a little? I prefer the order (a) Python built-in libraries, (b) third-party libraries (e.g. holidays), and finally (c) local imports (e.g. the cardinal imports), with newlines separating the categories.

If you don't want to add yourself to the CONTRIBUTORS file and would prefer I update the import order, just let me know and I'll go ahead and merge it.

Thanks again for the contribution - and for the tests around the behavior!

[ForeverEndeavor]

I can definitely do those things. Give me a few days as I've been busy with other items and I can push those changes for your final approval.

[ForeverEndeavor]

Re-ordered the imports and cleaned up the formatting a bit. I've neglected to add myself to the contributors file for now, although may do so in the future if that's ok with you.

[johnmaguire]

Thanks @ForeverEndeavor! I ended up ordering the imports in a separate commit and pulling over some of the cleanup but leaving out the larger changes. I'd like to keep the history relatively targeted, and I am old-school still using an 80-character line length rule. :)

Please feel free to come back and add your name to the CONTRIBUTORS file at any time!