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

fix: Consciencia negra day is not a national holiday #516

Closed
wants to merge 1 commit into from

Conversation

edniemeyer
Copy link
Contributor

@edniemeyer edniemeyer commented Jun 24, 2020

refs #

  • Tests with a significant number of years to be tested for your calendar.
  • Changelog amended with a mention describing your changes. Note Please do NOT change the version number here. It's the project maintainers' duty.

@brunobord
Copy link
Member

Thanks for this patch, but I wish it was a bit more elaborated in order to fulfill its purposes. Could you please provide:

  • a source that explains that this day is not a national holiday, since when, etc? I'm not sure that only switching from True to False would be enough if this day was a holiday and now isn't. There has been a few times in the past where we had to implement this kind of behaviour (a holiday that only appears since 2013, for example).
  • at least one test case function where you see that this holiday doesn't appear at the national level, but would appear only for States that include it, and not the others (I'm a bit puzzled that the fact that you're switching it off in your patch didn't trigger a FAIL in tests... looks like our coverage is not as great as it should be)

@brunobord
Copy link
Member

Of course, if you need help on this implementation, I'd be glad to provide it :o)

@edniemeyer
Copy link
Contributor Author

Sure! I would be happy to help! How can I add tests? Consciencia negra day is a holiday mandatory in some states, but not a national one. I've seen some of the states do have it setted correctly. Funny fact: carnival is said to be a non mandatory national holiday, even though it doesn't look like it in reality hahah. The BrazilBankCalendar class also I think should be the "official" Brazil class, which is usually what the companies follow over here, and what I used on my project.

Sources:

https://www.gov.br/pt-br/noticias/financas-impostos-e-gestao-publica/2020/01/portaria-divulga-feriados-e-pontos-facultativos-para-2020

https://g1.globo.com/economia/noticia/2020/01/01/veja-o-calendario-de-feriados-prolongados-e-de-pontos-facultativos-em-2020.ghtml

@brunobord
Copy link
Member

ok. I see. As I want to have this bug fixed quickly, I'll take your branch over and try to make a bugfix release as soon as possible.
Stay tuned!

@brunobord
Copy link
Member

(and I have time on my hands today, lucky you)

@brunobord
Copy link
Member

Version 10.2.0 was released and published on PyPI, including bugfixes for Brazil calendars.
Have fun upgrading!

@edniemeyer
Copy link
Contributor Author

Great!! Thanks!!

@brunobord
Copy link
Member

we aim to please :o)

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

2 participants