Closed Bug 1527513 Opened 6 years ago Closed 6 years ago

Slugid library returns bytestrings, not strings

Categories

(Taskcluster :: Services, enhancement)

enhancement
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: dustin, Assigned: jhford)

Details

Attachments

(1 file)

https://github.com/taskcluster/slugid.py/

In Python 3, slugid.v4() and slugid.nice() return bytes, which makes them a little hard to include in JSON output or other useful places. Since the results are always pure ASCII, it might be better to just return a string.

John or Pete, thoughts on how to solve this? Or even if it should be solved?

Flags: needinfo?(pmoore)
Flags: needinfo?(jhford)
Attached file patch

Since they're always within the ascii range, it's safe to store them as unicode or byte strings interchangably without memory overhead. I think it's reasonable to use the default string type for the version of Python being run.

Pete, if you agree and think this patch is good, I can land it.

Assignee: nobody → jhford
Status: NEW → ASSIGNED
Flags: needinfo?(jhford)
Attachment #9043621 - Flags: review?(pmoore)

I remember :aki did the work to make the library work under both python 2 and python 3, and at the time I recall he spent some time considering which types of string to use. I'm not much of a python expert, so I'll defer to him, as there may be reasons he chose bytes.

Flags: needinfo?(pmoore) → needinfo?(aki)
Attachment #9043621 - Flags: review?(pmoore) → review?(aki)

Reminder: if we update this in our repo, we may want to copy it to gecko too (it is vendored in firefox).

If I specified bytes, it may be because the original py2-only code was dealing with binary encodings, and I made it explicit without changing the behavior. Given a choice of behaviors, I would prefer ascii-only strings to be a string rather than a bytestring.

Flags: needinfo?(aki)
Comment on attachment 9043621 [details] patch Thanks!
Attachment #9043621 - Flags: review?(aki) → review+

This should result in a major version bump. I see a number of slugid.nice().decode('utf-8') in scriptworker tests that we'll need to fix up. Should be easy enough.

I've released Slug Id v2.0.0 on pypi.

Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED

(In reply to Aki Sasaki [:aki] from comment #6)

This should result in a major version bump. I see a number of slugid.nice().decode('utf-8') in scriptworker tests that we'll need to fix up. Should be easy enough.

Got tricked on my side as well, with code doing slugid.nice().decode() in taskcluster-github-decision and no pinned version.

You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: