Skip to content

Use a full unicode layer in preferences.py#45

Merged
dpinte merged 6 commits into
masterfrom
fix/unicode_preference
Mar 17, 2015
Merged

Use a full unicode layer in preferences.py#45
dpinte merged 6 commits into
masterfrom
fix/unicode_preference

Conversation

@dpinte
Copy link
Copy Markdown
Member

@dpinte dpinte commented Mar 17, 2015

PR #41 was changing the behaviour of the Preference class by not converting object to str anymore. This caused a clear problem when getting preferences from a file as the same call to retrieve a preference could either return a specific type or a unicode object as read by ConfigObj in the file.

This PR changes the behaviour again and gets back to the original design of Preferences which standardizes on str objects. When setting an object in the Preference class it is automatically converted to a unicode object (defaulting to utf-8 encoded strings as input), the unicode object is then properly serialized as utff-8 str by ConfigObj when saving the prefence to a file. When loading the preference in the preference helper, the object is nicely converted back to its target type, with Str as a special case where the unicode object is encoded to a utf-8 str)

This PR fixes the broken test suite.

@corranwebster Can you review? @rkern this changes a bit what we talked about in #41 but is probably the least painful implementation before switching to a better storage like yaml.

Fixes the failures described in #44.

Comment thread apptools/preferences/preferences.py Outdated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that if we get a str from the Python side, we should assume it to be ASCII, not UTF-8. We should not be throwing around UTF-8-encoded str objects in our Python code.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thinking about my initial use case a bit more, what I was trying to solve was the unicode object coming from the UI (or other sources) and that had to be serialized to the preferences. I agree that the assumption of utf-8 encoded string is not good.. I'll remove that.

@corranwebster
Copy link
Copy Markdown
Contributor

@rkern's comments are appropriate, as always. In addition, can you add this PR reference to this line in CHANGES.txt: https://github.com/enthought/apptools/blob/master/CHANGES.txt#L11

@dpinte
Copy link
Copy Markdown
Member Author

dpinte commented Mar 17, 2015

@rkern @corranwebster PR updated based on your comments. Does that look better?

@corranwebster
Copy link
Copy Markdown
Contributor

Actually, I meant #41 and #45 in the CHANGES.txt, but I'll fix it to my satisfaction :)

@corranwebster
Copy link
Copy Markdown
Contributor

LGTM.

There is a difference in behaviour in that now a str value will round-trip and come back as a unicode value (but one which tests as equal to the original value), but I think that's OK. If we don't want that behaviour, then that would add significant complexity to the code.

@dpinte
Copy link
Copy Markdown
Member Author

dpinte commented Mar 17, 2015

thanks @corranwebster. Merging.

dpinte added a commit that referenced this pull request Mar 17, 2015
Use a full unicode layer in preferences.py
@dpinte dpinte merged commit 1b83bea into master Mar 17, 2015
@dpinte dpinte deleted the fix/unicode_preference branch March 17, 2015 11:09
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

Successfully merging this pull request may close these issues.

3 participants