search

checkdomain / Holiday

Check whether a date is a holiday
www.checkdomain.de
MIT License
49 stars 26 forks source link

Refactor Easter date helpers #39

Closed hanzi closed 5 years ago

hanzi commented 6 years ago

This removes the AbstractEaster provider and replaces it with a dedicated helper class.

Semantically, having this as part of the class hierarchy didn't make a lot of sense to begin with, and I hope that my implementation is somewhat easier to parse for readers. It's also a bit more convenient with IDE auto completion.

Also, this implementation does not populate an entire array of largely unused dates anymore. Which I'm sure will free up lots of resources and reverse global warming.

wandersonwhcr commented 6 years ago

This PR modificates a lot of structure and can break some packages where classes extend AbstractEaster class.

FlorianKoerner commented 6 years ago

I like the idea. Since these are Breaking Changes, it would be a new major release.

@BenjaminPaap what do you think?

hanzi commented 6 years ago

I made these changes after critical voices in my other PR that touched the legacy Easter implementation.

If backwards compatibility is the main concern, I could remedy that by re-introducing the AbstractEaster class, marking it as @deprecated and just implementing the getEasterDates() method using EasterUtil.

There wouldn't be any uses of this class within the library, but potential provider implementations in 3rd party projects would not break. That would avoid a new major version (which would probably lead to most users being stuck with an old version until they bother checking their version constraints...)

BenjaminPaap commented 5 years ago

Thanks for your work @hanzi ❤️ .

Unfortunately I currently dont see any real benefit from these changes. Nearly every provider needs the calculation of the easter dates at the moment and therefore in my opinion it is irrelevant if we use an abstract class or a util class which is instantiated in every class.

Besides that I really dont like instantiating new instances in a class any more. This would make testing much harder if you need to change behaviour of that new instance.

BUT: I like the introduction of constants for the different dates which are relative to the easter date. Maybe we need another PR for that.

hanzi commented 5 years ago

Sorry, was on holiday and only saw this now.

Alright, I'll keep using this internally but I understand your concerns. Feel free to extract the constants part.