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
Raise maximum tag size to 96 #782
Raise maximum tag size to 96 #782
Conversation
udata/utils.py
Outdated
@@ -199,9 +200,10 @@ def recursive_get(obj, key): | |||
return recursive_get(value, parts) if parts else value | |||
|
|||
|
|||
def unique_string(length=None): | |||
def unique_string(length=36): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's name that magic number UUID_LENGTH
?
udata/utils.py
Outdated
'''Generate unique string''' | ||
string = str(uuid4()) | ||
# We need a string longer than length and an UUID is 36 chars length | ||
string = str(uuid4()) * int(math.ceil(length / 36.0)) | ||
return string[:length] if length else string |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
string
is the name of a standard module.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Which is not imported and as it's not a builtin, there is no collision.
I changed for uuid
but it's also a standard module and the result is not an uuid but just a string,
so I propose to keep string as it's explicit
udata/utils.py
Outdated
'''Generate unique string''' | ||
string = str(uuid4()) | ||
# We need a string longer than length and an UUID is 36 chars length | ||
string = str(uuid4()) * int(math.ceil(length / 36.0)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given our use-case, I would just add * 2
. Less clever but more readable :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
* 2
is not sufficient, we need at least 3.
But I spent too much time searching the cause of the test failure so lets make it work with any length.
Raise maximum tag length to at least support official INSPIRE tags