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: Update Irish bank holidays #639

Merged
merged 3 commits into from
Apr 2, 2022
Merged

Conversation

TeoTN
Copy link
Contributor

@TeoTN TeoTN commented Mar 9, 2022

Fixes #636

Copy link
Contributor

@javicalle javicalle left a comment

Choose a reason for hiding this comment

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

Some doubts about "Christmas Day"

holidays/countries/ireland.py Outdated Show resolved Hide resolved
@TeoTN
Copy link
Contributor Author

TeoTN commented Mar 16, 2022

Some doubts about "Christmas Day"

@javicalle Thanks, updated the PR with new commits to fix the issue

@TeoTN
Copy link
Contributor Author

TeoTN commented Mar 26, 2022

@javicalle Could you please review it?

Comment on lines 100 to 103
if self.observed and date(year, DEC, 26).weekday() == SAT:
self[date(year, DEC, 26) + rd(weekday=MON)] = name + " (Observed)"
elif self.observed and date(year, DEC, 26).weekday() == SUN:
self[date(year, DEC, 26) + rd(weekday=TUE)] = name + " (Observed)"
Copy link
Contributor

Choose a reason for hiding this comment

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

Now I understand this part.
I think that could be correct as is, just that maybe I would use:

        if self.observed and date(year, DEC, 26).weekday() in WEEKEND:
            self[date(year, DEC, 26) + rd(days=2)] = name + " (Observed)"

I'm not sure if it's more understandable or easier to maintain, so I'll leave it as a suggestion.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I like it being a little bit more verbose ;)

Copy link
Contributor

Choose a reason for hiding this comment

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

It's fine to me 👍🏻

Copy link
Contributor

@javicalle javicalle left a comment

Choose a reason for hiding this comment

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

The code part LGTM

@TeoTN
Copy link
Contributor Author

TeoTN commented Mar 31, 2022

How can we get it through the finishing line?

@javicalle
Copy link
Contributor

How can we get it through the finishing line?

That's up to @dr-prodigy

@dr-prodigy dr-prodigy changed the base branch from master to beta April 2, 2022 10:25
@dr-prodigy
Copy link
Collaborator

Sorry, been not available for some weeks! On this now, will let you know 👍

@dr-prodigy dr-prodigy merged commit 5b2ef00 into vacanza:beta Apr 2, 2022
@dr-prodigy
Copy link
Collaborator

Okay some fixes (tests, annotations, formatting) required, but then all done.
Released on beta branch, thank you both! 👍

@TeoTN
Copy link
Contributor Author

TeoTN commented Apr 13, 2022

Sweet, thanks a mil!

@sotatech
Copy link

Any progress with this, I'm still getting the same error on 2022.5.3.

Thanks.

@crowbarz
Copy link

There hasn't been a release of holidays since 15-Feb-2022, so the merged changes are still only present in the beta branch.

A workaround for Home Assistant (assuming that is what "2022.5.3" is referring to) involves copying the core workday integration into custom_components (see here for more info) and pinning the holidays dependency to 0.12, the last working version, in manifest.json. You'll need to also add a version key (any string will do) to the manifest as it is required by custom components since 2021.6.

@sotatech
Copy link

Thanks, I thought I was posting in HA Core Github, but obviously not! I'll try reverting holidays back to 0.12 as you suggest.

dr-prodigy added a commit that referenced this pull request Jun 5, 2022
Version 0.14
============

Released June 5, 2022

- Drop support for EOL Python 3.6 #328 (hugovk, dr-p)
- Package review #662 (dimbleby)
- Added financial markets support: ECB and NYSE, list_supported_financial() method (dr-p)
- Support for NY Stock Exchange #651, #458 (nadime, dr-p)
- Support for Malta #612, #630 (rafelbev)
- Support for Madagascar #656 (fav007)
- Support for Cyprus #410, #665 (digidestination, avnigo)
- Ireland as standalone country #636, #639 (TeoTN, dr-p, javicalle)
- Australia fixes #631 (jeremychrimes)
- Singapore updates #652 (mborsetti)
- Saudi Arabia fixes #642 (OsaydAbdu)
- Spain fixes #634 (javicalle)
- US fixes #648 (dashdrum)
- Greece fixes #659 (tudorvaran)
- India doc fixes #657 (dr-p)
- Poland fix #663 (kfsz)
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.

Ireland considering UK as base class and hence not being a country itself
5 participants