Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Similar Project #14

Closed
tinohager opened this issue May 27, 2017 · 10 comments
Closed

Similar Project #14

tinohager opened this issue May 27, 2017 · 10 comments

Comments

@tinohager
Copy link

Hi Martin

We have a similar project you are interested on a collaboration?

Best regards
Tino

@martinjw
Copy link
Owner

Hi @tinohager - yes, what's the project?

@tinohager
Copy link
Author

Hi Martin

This is my project https://github.com/tinohager/Nager.Date

Our two libraries have the first two places in the nuget charts for public holidays...

I have collected the public holidays for over 50 countries

@martinjw
Copy link
Owner

Impressive number of countries! I like the idea of a demo website too.

Nager.Date has some issues with the weekend-shifting, which is mostly done by Anglophone countries.

  • 4 July is always independence day in the USA.
  • But if it is on Saturday, there is a public holiday on Friday, and if it is on Sunday, there is a holiday on Monday.

Your library shows 4 July always, while this library shows the weekday holiday. Both are strictly correct... (You do the adjustment for Veteran's Day, but not others).

There are some interesting differences in approach- you pass in a country-code enum and have private country providers, which I instantiate specific public country providers (new FrancePublicHoliday()).

How would you like to collaborate? I guess we could merge the two by replacing Nader.Date country providers with PublicHoliday country instances?

@tinohager
Copy link
Author

My consideration is to continue just one project. We can also start a completely new one but then we lose the previous spread.

I think we would have to modify the providers of "Nager.Date" only a little bit to incorporate your logic. You have a very good documentation and good tests.

What could you imagine for a co-operation?

@martinjw
Copy link
Owner

Here are some things the differences between PublicHoliday and Nager.Date.

  • Multi-targeting. PH is netstandard1.3;net35;net40;net46, ND is netstandard1.1 only (=net45). Easy to do in the new 2017 csproj (the only complication is the net35 target, which breaks the dotnet command line).
  • PH has VS2015 and VS2017 projects (different types of csproj), ND is VS2017 only. In businesses, VS2017 is still new, I still get pull requests for the VS2015 version. This only affects development and over time VS2015 will diminish.
  • PH creates a country provider directly (new FrancePublicHoliday()), rather than hidden in a factory. The obvious solution is to allow both. It also makes regional/state differences much easier to handle (discussed later)
  • ND always builds all holidays in a year. PH is optimised for the IsPublicHoliday(date) case (based on personal experience!).
  • PH returns the PublicHoliday class. PH returns list of dates, except for names which come in a dictionary. I think an overload for holidays just with dates is definitely required. For holiday names a better structure is required. The PublicHoliday.Name and LocalName properties are too simplistic (countries can have multiple languages and multiple names for holidays).
  • PH has NextWorkingDay and PreviousWorkingDay calculations. Very useful for scheduling.
  • ND has IsOfficialPublicHolidayByCounty and IsAdministrationPublicHolidayByCounty.
    • Not clear about the difference between Official and Administration, although I know some countries have special bank and/or federal holidays that don't apply to most businesses, and there's the Swedish "eve" system of half-days.
    • PublicHoliday.Counties is problematic. The "County" designation isn't good; the correct name differs by country (province, state, territory...). I think country providers should have country specific names (USAPublicHoliday has states), the generic name should be something like "region" or "subdivision"? Just to make it even more complicated, there are local holidays (Royal Hobart Regatta).
  • ND has PublicHoliday.LaunchYear. I don't think providing the launch year is terribly useful.

I quickly tried to put the two projects together, simply by making ND dependent on PH. it works, but there's a nasty naming collision ("PublicHoliday"). Here's USA:

namespace Nager.Date.PublicHolidays
{
    using PublicHoliday = Model.PublicHoliday;
    public class UnitedStatesProvider : IPublicHolidayProvider
    {
        public IEnumerable<PublicHoliday> Get(int year)
        {
            var usa = new global::PublicHoliday.USAPublicHoliday();
            var hols = usa.PublicHolidayNames(year);
            var items = new List<PublicHoliday>();
            var countryCode = CountryCode.US;
            foreach (var hol in hols)
            {
                var dt = hol.Key;
                var name = hol.Value;
                items.Add(new PublicHoliday(year, dt.Month, dt.Day, name, name, countryCode));
            }
            return items.OrderBy(o => o.Date);
        }

Based on this, I think the first thing to do is to refactor the projects rather than immediately put them into one project. Specifically, come up with common contracts - your PublicHoliday class v2.

Sounds good?

@tinohager
Copy link
Author

Hi

  1. I think is only a little change on the project config to change to .netstandard 1.3
  2. Yes i have update to visual studio 2017 but is no problem to add a second solution file for VS2015
  3. Okay - discussed later :)
  4. I think is a short performance optimizing
  5. I have check a lot of wikipedia sites and the most have english name and local name (https://en.wikipedia.org/wiki/Public_holidays_in_Russia, https://en.wikipedia.org/wiki/Public_holidays_in_Germany, https://en.wikipedia.org/wiki/Public_holidays_in_France)
  6. We can add this mehtods i think is a good idea
    7.1) This is added by a pull request Updated Nager.Date to handle county CH-NE holidays nager/Nager.Date#1
    7.2) I am open to change it
  7. LaunchYear we can remove i cannot found this information for every year

I've also thought about it to add a type enum to the holidays (bankholiday, school, ...) but is not easy to set the type

@jcron
Copy link

jcron commented Sep 29, 2017

Hi @tinohager @martinjw - I am looking at both of these libraries for something we may be working on in the next couple of weeks. I see that both have had recent updates - did you all come to an agreement on merging this work into one project? We are still deciding whether to build our own implementation and store these dates in a database so our customers could modify or use a library like one of these.

@martinjw
Copy link
Owner

First stage is to make the libraries compatible. This library will have a refactor (v2) so it's contracts are compatible with Nager.Date, and it can function as a lower level library. I haven't had the time to do this, but I will get back to it eventually.

@tinohager
Copy link
Author

tinohager commented Sep 30, 2017

I have compared the two projects country support again, in the current state, PH have two countries supported there are not available in ND (Kazakhstan and Japan). I think the the great idea is not merge Code, but rather the more people work on one project. It is very much work to create a new country and write tests, every year check whether something has changed...

@jcron if you start again a new project all of the hard work, is lost again... you are welcome to work on a existing project

My goal is a blue world https://date.nager.at/

@tinohager
Copy link
Author

Hi Martin, What does the idea actually look like? I have in the meantime moved my project to an organization you would like to join here?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants