Skip to content

Fixing unicode support for PreferencesHelper#41

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

Fixing unicode support for PreferencesHelper#41
dpinte merged 11 commits into
masterfrom
fix/unicode_preference

Conversation

@dpinte
Copy link
Copy Markdown
Member

@dpinte dpinte commented Mar 4, 2015

The PreferencesHelper don't work properly when getting/storing unicode strings with non-ascii caracters. This PR intends to fix the issue.

@dpinte dpinte changed the title Fixing unicode support for PreferenceHelper Fixing unicode support for PreferencesHelper Mar 4, 2015
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.

Man, that's very unreliable.

Can we just make ConfigObj(file_or_filename, encoding='utf-8') instead? What breaks when we do that?

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.

that's a great suggestion. I'll look into that as a better solution.

@dpinte
Copy link
Copy Markdown
Member Author

dpinte commented Mar 4, 2015

@rkern changing only ConfigObj is not enough as the Preferences class convert all the object to str internally. Either we need to change the way the preference sets the object by not converting them to str and letting ConfigObj doing the conversion job or we need to do the proper conversion from str to unicode with a decode/encode. What do you think? Changing the way the preference work by not converting the object to str is not hard. I am just slightly worried on the impact on external users.

@dpinte
Copy link
Copy Markdown
Member Author

dpinte commented Mar 4, 2015

@rkern 1f343e3 contains the other implementation. All the tests are passing with those changes. What do you think? 1f343e3 or 585c88b ?

@dpinte
Copy link
Copy Markdown
Member Author

dpinte commented Mar 5, 2015

@rkern I've updated the comments and the tests to do a full load/save cycle.

@dpinte
Copy link
Copy Markdown
Member Author

dpinte commented Mar 5, 2015

Fixes #35

@rkern
Copy link
Copy Markdown
Member

rkern commented Mar 6, 2015

LGTM! Thanks!

dpinte added a commit that referenced this pull request Mar 6, 2015
Fixing unicode support for PreferencesHelper
@dpinte dpinte merged commit d14d22b into master Mar 6, 2015
@dpinte dpinte deleted the fix/unicode_preference branch March 6, 2015 13:26
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.

2 participants