Introduce custom exceptions [WIP]

[arkid15r]

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)
• Breaking change (a code change causing existing functionality to break)
• New feature (new python-holidays functionality in general)

Checklist

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

[coveralls]

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

[PPsyrius]

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}""

[KJhellico]

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

[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 😛

[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 😛

Yeah, I forgot that exists somehow 💀

[arkid15r]

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.

[KJhellico]

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.

[PPsyrius]

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'?

[arkid15r]

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 👍

[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.

[sonarcloud]

SonarCloud Quality Gate failed.   

2 Bugs
0 Vulnerabilities
0 Security Hotspots
12 Code Smells

No Coverage information
0.0% Duplication

Catch issues before they fail your Quality Gate with our IDE extension SonarLint