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

Should uuid.UUID() accept another UUID() instance? #76293

Closed
mjpieters mannequin opened this issue Nov 22, 2017 · 8 comments
Closed

Should uuid.UUID() accept another UUID() instance? #76293

mjpieters mannequin opened this issue Nov 22, 2017 · 8 comments
Labels
type-feature A feature request or enhancement

Comments

@mjpieters
Copy link
Mannequin

mjpieters mannequin commented Nov 22, 2017

BPO 32112
Nosy @mjpieters, @merwok, @serhiy-storchaka, @ztane, @iritkatriel

Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

Show more details

GitHub fields:

assignee = None
closed_at = <Date 2020-12-19.00:23:12.119>
created_at = <Date 2017-11-22.10:52:36.660>
labels = ['type-feature']
title = 'Should uuid.UUID() accept another UUID() instance?'
updated_at = <Date 2020-12-19.00:23:12.118>
user = 'https://github.com/mjpieters'

bugs.python.org fields:

activity = <Date 2020-12-19.00:23:12.118>
actor = 'iritkatriel'
assignee = 'none'
closed = True
closed_date = <Date 2020-12-19.00:23:12.119>
closer = 'iritkatriel'
components = []
creation = <Date 2017-11-22.10:52:36.660>
creator = 'mjpieters'
dependencies = []
files = []
hgrepos = []
issue_num = 32112
keywords = []
message_count = 6.0
messages = ['306719', '306722', '306728', '306909', '306910', '377949']
nosy_count = 5.0
nosy_names = ['mjpieters', 'eric.araujo', 'serhiy.storchaka', 'ztane', 'iritkatriel']
pr_nums = []
priority = 'normal'
resolution = 'rejected'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'enhancement'
url = 'https://bugs.python.org/issue32112'
versions = []

@mjpieters
Copy link
Mannequin Author

mjpieters mannequin commented Nov 22, 2017

When someone accidentally passes in an existing uuid.UUID() instance into uuid.UUID(), an attribute error is thrown because it is not a hex string:

>>> import uuid
>>> value = uuid.uuid4()
>>> uuid.UUID(value)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/Users/mjpieters/Development/Library/buildout.python/parts/opt/lib/python2.7/uuid.py", line 133, in __init__
    hex = hex.replace('urn:', '').replace('uuid:', '')
AttributeError: 'UUID' object has no attribute 'replace'

This happened in the Stack Overflow question at https://stackoverflow.com/q/47429929/100297, because the code there didn't take into account that some database drivers may already have mapped the PostgreSQL UUID column to a Python uuid.UUID() object.

The fix could be as simple as:

    if hex is not None:
        if isinstance(hex, uuid.UUID):
            int = hex.int
        else:
            hex = hex.replace('urn:', '').replace('uuid:', '')
            hex = hex.strip('{}').replace('-', '')
            if len(hex) != 32:
                raise ValueError('badly formed hexadecimal UUID string')
            int = int_(hex, 16)

Or we could add a uuid=None keyword argument, and use

    if hex is not None:
        if isinstance(hex, uuid.UUID):
            uuid = hex
        else:
            hex = hex.replace('urn:', '').replace('uuid:', '')
            hex = hex.strip('{}').replace('-', '')
            if len(hex) != 32:
                raise ValueError('badly formed hexadecimal UUID string')
            int = int_(hex, 16)
    if uuid is not None:
        int = uuid.int

@ztane
Copy link
Mannequin

ztane mannequin commented Nov 22, 2017

I've been hit by this too, in similar contexts, and several times. It is really annoying that it is easier to coerce an UUID or UUID-string to a string than to coerce to a UUID. Usually when the copy semantics are clear and the class is plain old data, Python lets you execute the constructor with an instance of the same class:

    >>> bytes(bytes())
    b''
    >>> bytearray(bytearray())
    bytearray(b'')
    >>> int(int())
    0
    >>> complex(complex())
    0j
    >>> tuple(tuple())
    ()

I don't to see why this shouldn't be true with UUID as well.

@serhiy-storchaka
Copy link
Member

Not always the constructor accept an instance of the same class. I'm sure that this is not true in the majority of cases.

The constructor of int accepts an instance of int and the constructor of tuple accepts an instance of tuple because the constructor of int accepts an arbitrary real number, and the constructor of tuple accepts an arbitrary iterable, and int and tuple are a real number and an iterable correspondingly. There is no reason to forbid accepting an instance of the same class in these cases.

In contrary, the UUID constructor accepts a hexadecimal string, but UUID itself is not a hexadecimal string. Similarly the range constructor doesn't accept a range instance, and the file constructor doesn't accept a file instance.

>>> range(range(3))
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: 'range' object cannot be interpreted as an integer
>>> io.FileIO(io.FileIO('/dev/null'))
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: expected str, bytes or os.PathLike object, not _io.FileIO

For converting an existing UUID instance to UUID you can can first convert it to str:

    newvalue = uuid.UUID(str(oldvalue))

Or better avoid the conversion at all.

    if isisnstance(oldvalue, uuid.UUID):
        newvalue = oldvalue
    else:
        newvalue = uuid.UUID(oldvalue)

@merwok
Copy link
Member

merwok commented Nov 24, 2017

I don’t see a downside in accepting the feature request here.

Maybe ask on python-ideas to get more feedback?

@serhiy-storchaka
Copy link
Member

Growing the size of the code and the documentation, complicating the mental model, slowing down all other cases, the risk of introducing bugs. This is the cost that we should pay for adding a new feature. The benefit of adding a feature should be larger than this cost. I'm not sure this is a case. But if you think it is worth, you can add it.

Note that by adding this feature you can open a can of worms and will need to add support of time(time), date(date), namedtuple(namedtuple), etc for the same reason, because some database drivers already do mapping for you.

@iritkatriel
Copy link
Member

I agree with Serhiy that there needs to be a reason for adding this to a class constructor.

In the case of int, this is to implement conversion - it's not accepting int, it's accepting any number (it would be weird of int(3.4) worked and int(3) didn't). Similarly, complex constructor accepts numbers.

In the case of tuple, bytes, bytearray - the constructor accepts any iterable, again in order to implement type conversion.

So it's not random - these constructors accept objects of the same type because they accept params of a more generic type, in order to provide type conversion functionality.

What functionality would we gain from increasing the API surface of the UUID constructor as you suggest?

@iritkatriel iritkatriel added the type-feature A feature request or enhancement label Dec 19, 2020
@ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
@erny
Copy link

erny commented Apr 26, 2022

Would it be good to raise another error than "AttributeError", something like "TypeError"? The class expects a string as its first argument, but doesn't check its type.

@merwok
Copy link
Member

merwok commented Apr 26, 2022

What functionality would we gain from increasing the API surface of the UUID constructor as you suggest?

This would make it easier to write code that works at boundaries and needs to convert a value that could be a string (from a file or another external source) or a UUID (already parsed somewhere else).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type-feature A feature request or enhancement
Projects
None yet
Development

No branches or pull requests

4 participants