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

Add typings and error handling - use your library in Home Assistant #260

Open
Misiu opened this issue Jan 12, 2025 · 8 comments
Open

Add typings and error handling - use your library in Home Assistant #260

Misiu opened this issue Jan 12, 2025 · 8 comments

Comments

@Misiu
Copy link
Contributor

Misiu commented Jan 12, 2025

I'm trying to replace the old and unsupported whois package with your package in Home Assistant (home-assistant/core#135297), but I got a comment against it - home-assistant/core#135297 (review)

Your library was used before and was replaced with the currently unsupported library back in 2022 - home-assistant/core#63227

I would like to hear your opinion about adding typing and error handling to your package.

I saw a helper class that was calling your package and it throws more error based on the response - https://github.com/mkrasowski/homeassistant-core/blob/replace-unmaintained-whois-library/homeassistant/components/whois/helper.py#L36

Is adding exceptions like https://github.com/mboot-github/WhoisDomain/blob/master/whoisdomain/exceptions.py an option?
Sadly the current whois integration in Home Assistant doesn't work well for PL domains (and probably other domains) and doesn't return the expiration date.

@richardpenman
Copy link
Owner

hi there, thanks for reaching out.

If someone sends a PR for type hints and/or improved error handling, that would be great! Or I could add this within the next few weeks.

Using those more fine grained exception classes would certainly be an improvement. Currently there is just a single PywhoisError exception type.

@Misiu
Copy link
Contributor Author

Misiu commented Jan 13, 2025

@richardpenman thank you for the reply. I'm not a Python developer but I can try to add error handling
What do you think about the second requirement from home-assistant/core#135297 (review)?

It lacks typing and is missing error handling.

I'm not sure if creating a class like https://github.com/mboot-github/WhoisDomain/blob/master/whoisdomain/domain.py will be enough.
Any thoughts on that?

@richardpenman
Copy link
Owner

OK, I'll add some type hints and different exception classes, then see what the feedback is.

@Misiu
Copy link
Contributor Author

Misiu commented Jan 14, 2025

I've started adding something (Misiu@e1e90df) but I don't feel too comfortable at adding types.

I'm going to take a look at the tests and will get back here with my findings, generally, when trying to parse datetime, there is no error, and you return text as it is. When I tried to add an proper error, some tests failed, for example with 2002-10-22 17:48:23 CLST parsing

@richardpenman
Copy link
Owner

I have added type hints.
Yes currently the default is to leave the date string as is when no known format matches.

@Misiu
Copy link
Contributor Author

Misiu commented Jan 26, 2025

I've added some errors and fixed the date parsing a bit.
https://github.com/Misiu/python-whois/tree/more-errors
what do you think about my changes?
if you approve them I can work on adding more errors.

@richardpenman
Copy link
Owner

@Misiu this is an excellent improvement - thanks very much for working on this!

Did you want to send this as a PR now or first work on the other exceptions like WhoisQuotaExceeded?

@Misiu
Copy link
Contributor Author

Misiu commented Jan 27, 2025

@richardpenman PR created - #267

I've tried my best to make this backward compatible. The errors extend PywhoisError, so we should be good on that, but the date parsing logic is a breaking change because instead of returning the original string I'm throwing WhoisUnknownDateFormatError.
data_preprocessor is a new, optional parameter, it helped me solve the problems with passing tests.
I'm learning Python, so a good review is welcome :)

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

No branches or pull requests

2 participants