Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Empty string should not raise ValueError on construction #45

Open
shane-kearns opened this issue Mar 4, 2024 · 1 comment · May be fixed by #46
Open

Empty string should not raise ValueError on construction #45

shane-kearns opened this issue Mar 4, 2024 · 1 comment · May be fixed by #46

Comments

@shane-kearns
Copy link

A ValueError is raised here if passing the empty string '' since it is a false value.

if not (fqdn and isinstance(fqdn, str)):

The correct line should read:

        if not isinstance(fqdn, str):
            raise ValueError("fqdn must be str")

Since isinstance(None, str) is valid python and returns False.

Which gives the following behaviour that I think is correct:

Python 3.12.2 (tags/v3.12.2:6abddd9, Feb  6 2024, 21:26:36) [MSC v.1937 64 bit (AMD64)] on win32
Type "help", "copyright", "credits" or "license" for more information.
>>> from fqdn import FQDN
>>> a = FQDN('')
>>> a.is_valid
False
>>> b = FQDN()
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: FQDN.__init__() missing 1 required positional argument: 'fqdn'
>>> c = FQDN(None)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "C:\Users\shane.kearns\Documents\git\prappspec\tests\.virtualenv\Lib\site-packages\fqdn\__init__.py", line 44, in __init__
    raise ValueError("fqdn must be str")
ValueError: fqdn must be str
shane-kearns pushed a commit to shane-kearns/fqdn that referenced this issue Mar 4, 2024
The empty string is already treated as invalid by the supplied
validators. It was incorrect to raise ValueError on construction since
this requires users to pre-validate strings before passing to a
validation library.

Fixes ypcrts#45
@ypcrts
Copy link
Owner

ypcrts commented Mar 4, 2025

This changes the API contract. So it could be a breaking change for other users.

Should we introduce this? Why?

Can you explain why the empty string should be a valid FQDN?

# for free to join this conversation on GitHub. Already have an account? # to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants