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

More brazilian holidays ibge #409

Closed

Conversation

luismalta
Copy link
Contributor

@luismalta luismalta commented Oct 25, 2019

This PR does the following:

  • Iso_register decorator accepts more than one parameter
  • Add more holidays in brazilian states and cities
  • Add IBGE codes to iso_register for brasilian states and cities holidays
  • Add script to get all brazilian holidays with their respective type and coverage

# subregion code not in region_registry
code = code_elements[0]

subregion = None
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is gonna work for more than 2 arguments?

@@ -81,4 +81,4 @@ def test_germany_subregion(self):

def test_slovenia_code(self):
# Source: https://github.com/peopledoc/workalendar/pull/291
self.assertEqual(registry.region_registry['SI'], Slovenia)
self.assertEqual(registry.region_registry[('SI',)], Slovenia)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You cannot keep this interface intact?

(5, 20, "Aniversário de Palmas"),
)


class BrazilBankCalendar(Brazil):
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you check this information: https://feriadosbancarios.febraban.org.br

@luismalta
Copy link
Contributor Author

This PR is ready to be reviewed

Copy link
Member

@brunobord brunobord left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

First of all, it looks like a big contribution, and you have spent a lot of time on it... so thanks a bunch, you have delivered a tremendous work, I'm very happy each time I see people spending their time to improve the library.

With that said, I must say that I'm confused by the iso_register change. By definition, this should be an ISO register, that is to say a mapping for ISO codes vs. their corresponding classes.
As far as I can say, the IBGE is a Brazilian administration, and their codes are not ISO codes. As a consequence, I don't think that I would want to add IBGE codes to the ISO register.

I don't think also it would be a good idea to allow every national geographic notation to be included into the ISO register. This project focus is about providing calendar classes and tools to compute holidays / working days.

But your present PR provides a huge improvement for Brazilian calendars, so I would consider a few changes to be integrated in the upstream code:

  • get rid of the IBGE-related changes, that is to say the change in the function signature to allow multiple codes to be provided and by consequence, the IBGE codes added to the Brazilian classes.
  • I'm also challenging the point of the workalendar/brazil_holidays_set.py file

All the rest of this Pull-Request looks awesome and I would be delighted to see it in workalendar: the more calendars we have, the more users ;o)

]


def brazil_all_holidays_set(year):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what's the point of this module? I don't see it used elsewhere in the code and why it would be included in the library.

@luismalta
Copy link
Contributor Author

Hi @brunobord , thank you for the review. Really the file brazil_holidays_set.py doesn't make sense to be in lib, sorry about that. Along with the IBGE code, the idea of adding it was to be able to have a way to retrieve city calendars directly and since IBGE is a Brazilian institute responsible for generating this kind of geographical statistics I thought it was the best solution. But really, as it is still not an ISO code, it makes no sense that it is there. Would you suggest any solutions to this problem?

@brunobord
Copy link
Member

brunobord commented Nov 3, 2019

Would you suggest any solutions to this problem?

There are several answers to this question.

The clearest and simplest path that I see is to simply let this part out of workalendar and take this load in your 3rd party app (as an integrator). You may want to declare a mapping between the IBGE codes and their corresponding calendar classes. As far as I can say, you only need a dictionary like this:

IBGE_REGISTRY = {
    "BR-IBGE-12": BrazilAcre,
    "BR-IBGE-27": BrazilAlagoas,
    "BR-IBGE-16": BrazilAmapa,
# etc, etc, etc
}

This mapping doesn't have to exist in workalendar, it just has to exist in your target application and you may be free to adapt it to your needs (add/remove as many entries as you want in it).

Now, there's a thin line where it might be possible to integrate this mapping to the workalendar library, also. The definition of this dictionary could be added to the brazil.py file and might be imported just as a property of the module:

from workalendar.america.brazil import IBGE_REGISTRY
calendar_class = IBGE_REGISTRY["BR-IBGE-3205309"]
vitoria_city = calendar_class()

I'm not really in favor of this latter solution, because it adds something like a quirks, or a very Brazil-specific variable in this module. And if we do that for Brazil, why would we wouldn't do that for the rest of the world and integrate every geographical notation for other countries or zones? :/

Eventually, if we can find other workalendar users that would like to have this sort of mapping integrated, why not? but as far as I can tell, the ISO register is already a solid tool that relies on an international standard, and I'd be more comfortable if it stayed that way.

Did I answer your question?

@mileo
Copy link

mileo commented Nov 5, 2019

Hi @brunobord, thanks for your review.

I conducted a brief survey of how cities are classified worldwide and found some standards:
1 - ISO 3166-1 / ISO 3166-2 already used by workalendar, but there is no city data.
2 - Locode http://www.unece.org/cefact/locode/welcome.html (download the CSV here);
3 - NUTS, That I believe you already know. It's hold at level 3 the cities.

All of these datasets are excellent and work for their purposes. However they are generally of no use when trying to address a local problem, for example NUTS although excellent is not implemented in US cities.

As I see it, the purpose of the library is to keep an up-to-date list of all city holidays.

So it is extremely important that it allows local users to be able to access its interface clearly and simply.

Because only the local action of these users will be able to keep the holiday list up to date so that global users can use it.

So in my opinion the best way to keep the library active would be to allow users to:
1 - Find holidays by local standards, such as IBGE in Brazil;
2 - Determine the other patterns of a particular region;

For example:

If the holiday, "Armistice Day, called L'Armistice de la Première Guerre Mondiale", was not a national holiday and only a city holiday in Paris.

It would be great that when querying the city of paris through INSEE, Institut national de la statistique et des études économiques, code. But is algo great to get NUTS code too (FR101 - Paris) of this record and vice versa.

In this example we allow the French to contribute to the library, as well as all of Europe and the world to use it.

Best regards

@brunobord
Copy link
Member

It would be great that when querying the city of paris through INSEE, Institut national de la statistique et des études économiques, code. But is algo great to get NUTS code too (FR101 - Paris) of this record and vice versa.

It seems that what you are describing here is more the case of an external library that would index calendars in order to have a search engine able to query this index.
Something like a "workalendar search engine".

But this multiple-criteria search engine is not really in the scope of workalendar, from my point of view. The purpose of workalendar is to provide tools to compute holidays in as many areas as possible. How users and integrators would interact with it is more about their application business logic.

So it is extremely important that it allows local users to be able to access its interface clearly and simply.

I don't see how the current existing module doesn't allow that. Maybe in the brazil.py file, that'd be convenient to add an __all__ variable that'd list the available calendars, so one could do from workalendar.america.brazil import __all__, for example.

But I see that it looks like the IBGE reference is very important and it wouldn't be a problem to have it into the Brazilian module. Even if I'm not really enthusiastic about it ;o)

I'll work on it today, and I may ping both of you for a review, and we'll see if the compromise fits. I'll fork from this PR, so all of your valuable work will definitely be part of this project, because... I love when people from around the world are contributing to workalendar with such great value. ❤️

Stay tuned!

@brunobord brunobord mentioned this pull request Nov 8, 2019
2 tasks
brunobord added a commit that referenced this pull request Nov 8, 2019
- Added 27 Brazil calendars -- thanks a lot to @luismalta & @mileo, (#409 & #415),
- Fixes and additions to some calendars -- again, thanks to @luismalta & @mileo, (#409 & #415)
- Added an IGBE_REGISTER to reference IGBE (brazilian) calendars with related tests (#415).
brunobord added a commit that referenced this pull request Nov 15, 2019
- Added 27 Brazil calendars -- thanks a lot to @luismalta & @mileo, (#409 & #415),
- Fixes and additions to some calendars -- again, thanks to @luismalta & @mileo, (#409 & #415)
- Added an IGBE_REGISTER to reference IGBE (brazilian) calendars with related tests (#415).
brunobord added a commit that referenced this pull request Nov 15, 2019
**New calendars**

- Added 27 Brazil calendars -- thanks a lot to @luismalta & @mileo, (#409 & #415)

**Enhancements**

- Added compatibility with Python 3.8 (#406).
- Added an IBGE_REGISTER to reference IBGE (brazilian) calendars with related tests (#415).
- Improve ISO registry interface by raising an error when trying to register a non-Calendar class (#412).

**Other changes**

- Fixes and additions to some Brazil calendars ; again, thanks to @luismalta & @mileo, (#409 & #415)
- Fix Denmark, re-add Christmas Eve, which is widely treated as public holiday ; thx to @KidkArolis, (#414).
- Increase Malaysia coverage by adding tests for missing Deepavali & Thaipusam.
- Increase China coverage by adding tests for special extra-holidays & extra-working days cases.
@brunobord
Copy link
Member

version 7.1.0 was released and published on PyPI including your very valuable contribution to the Brazilian calendars. Happy upgrade!

@mileo mileo deleted the more_brazilian_holidays_ibge branch November 15, 2019 13:56
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

Successfully merging this pull request may close these issues.

None yet

3 participants