python-holidays Introduce custom exceptions [WIP]

Proposed change

This PR Introduces PH custom exceptions. The goal is to share it early in order to gather naming and use case suggestions. This is a breaking change. The documentation updates are coming soon.

Type of change

[ ] New country/market holidays support (thank you!)
[ ] Supported country/market holidays update (calendar discrepancy fix, localization)
[ ] Existing code/documentation/test/process quality improvement (best practice, cleanup, refactoring, optimization)
[ ] Dependency upgrade (version update)
[ ] Bugfix (non-breaking change which fixes an issue)
[x] Breaking change (a code change causing existing functionality to break)
[ ] New feature (new python-holidays functionality in general)

Checklist

[x] I've followed the contributing guidelines
[x] This PR is filed against beta branch of the repository
[x] This PR doesn't contain any merge conflicts and has clean commit history
[x] The code style looks good: make pre-commit
[x] All tests pass locally: make test, make tox (we strongly encourage adding tests to your code)
[x] The related documentation has been added/updated (check off the box for free if no updates is required)

Jun 22 '23 02:06 arkid15r

coverage: 100.0%. remained the same when pulling dce3b178d3ec93f450c3be2d06d02d34d517815d on arkid15r:add-custom-exceptions into 940b6b807588c0cc835b1966a229175b00eb3f75 on dr-prodigy:beta.

Jun 22 '23 02:06 coveralls

LGTM, though we might want to add actionable suggestions on top of existing errors as the ones slowly rolled out by the Python Foundation since 3.10 release.

For example, rather than:

f"Country '{self.country}' does not have subdivision " f"'{subdiv}'"

We might want to change to:

f"Country '{self.country}' does not have subdivision " f"'{subdiv}'. Consider consulting f"{github}" for available subdivision list."

Or even better, get this to read from readme.rst file somehow:

f"Country '{self.country}' does not have subdivision " f"'{subdiv}'. Available subdivisions for this country: f"{list}""

Jun 22 '23 05:06 PPsyrius

Or even better, get this to read from readme.rst file somehow:

f"Country '{self.country}' does not have subdivision " f"'{subdiv}'. Available subdivisions for this country: f"{list}""

self.subdivisions 😛

Jun 22 '23 11:06 KJhellico

Or even better, get this to read from readme.rst file somehow:

f"Country '{self.country}' does not have subdivision " f"'{subdiv}'. Available subdivisions for this country: f"{list}""

self.subdivisions 😛

Yeah, I forgot that exists somehow 💀

Jun 22 '23 11:06 PPsyrius

We might want to change to:

f"Country '{self.country}' does not have subdivision " f"'{subdiv}'. Consider consulting f"{github}" for available subdivision list."

So the general idea is to make the error messages more descriptive, right?

I continue to have some doubts about the need for exceptions for Israel and (especially) Japan, but I like the idea of custom exceptions. 👍

This can be addressed separately, however I'd like to hear more on why the year out of range exception makes you being doubtful.

Jun 22 '23 15:06 arkid15r

This can be addressed separately, however I'd like to hear more on why the year out of range exception makes you being doubtful.

These two countries are not so "exceptional" that a special scheme should be applied to them. 🙂

Start year - as for many other countries it's a year when current holidays list was established. We typically use if year <= N: return None for that. End year - for Israel, it may be an exception (due to used calendar implementation, we really have no holidays data for later years). For Japan, I see no reason to limit. I think it's left from the time when observed holidays were hardcoded.

Jun 22 '23 15:06 KJhellico

So the general idea is to make the error messages more descriptive, right?

Right, same goes for cases where user's country input doesn't exists due to typo i.e. Lybia instead of Libya - in ideal scenario the code would try to point out what they might meant to type after the initial exception message:

Did you mean 'Libya'?

Jun 22 '23 16:06 PPsyrius

Start year - as for many other countries it's a year when current holidays list was established. We typically use if year <= >N: return None for that. End year - for Israel, it may be an exception (due to used calendar implementation, we really have >no holidays data for later years). For Japan, I see no reason to limit. I think it's left from the time when observed holidays >were hardcoded.

My plan is to introduce start/end year control later and raise or warn when user requests holidays out of range of supported years (instead of returning an empty dict). Both start/end year range would be optional and enforced if they were set only.

So I guess YearOutOfRangeError can be ignored/removed for now.

Right, same goes for cases where user's country input doesn't exists due to typo i.e. Lybia instead of Libya - in ideal >scenario the code would try to point out what they might meant to type after the initial exception message:

That's a good user experience suggestion. Eventually we'd like to have this implemented 👍

Jun 28 '23 16:06 arkid15r

I removed YearOutOfRangeError. As I mentioned it's early shared changes which I'm going to target to v0.30. Please take another look -- any suggestions are welcome.

Jul 11 '23 01:07 arkid15r

SonarCloud Quality Gate failed.