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

Ensure UTF8 encoding #7

Merged
merged 1 commit into from
Sep 26, 2014
Merged

Conversation

dgilm
Copy link
Contributor

@dgilm dgilm commented Sep 25, 2014

Avoid future problems with database fields encoded with latin1 and other codifications. This fixes a livestatus error (shinken-monitoring/mod-livestatus#33) creating a livestatus response with fields imported with this module encoded as latin1.

Avoid future problems with database fields encoded with latin1 and other codifications. This fixes a livestatus error (shinken-monitoring/mod-livestatus#33) creating a response with fields imported with this module encoded as latin1.
Seb-Solon added a commit that referenced this pull request Sep 26, 2014
Enh : Ensure UTF8 encoding
@Seb-Solon Seb-Solon merged commit 7bcfdee into shinken-monitoring:master Sep 26, 2014
@Seb-Solon
Copy link
Contributor

Thanks!

Same suggestion for this module about tests :)

@dgilm dgilm deleted the patch-4 branch September 26, 2014 15:01
@naparuba
Copy link
Member

Hi,

I'm not ok with this patch. It import chardet that is not a default module. It add a new dep for a very specific case. So it should be at least optional and called only if need, and if possible avoid even with a new module parameter (like let the user set the charset if it's not utf8 for example).

@@ -30,6 +30,8 @@
except ImportError:
MySQLdb = None

import chardet
Copy link
Member

Choose a reason for hiding this comment

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

PB here :)

@dgilm dgilm restored the patch-4 branch September 29, 2014 11:13
@dgilm
Copy link
Contributor Author

dgilm commented Sep 29, 2014

I didn't know that chardet isn't a default module although is installed in all my systems without extra actions. Sorry about that.

In that case @naparuba, I should have check if chardet is installed before using it. Check out the following patch:
dgilm@a36ea5a

I can prepare a new pull request if you want.

@naparuba
Copy link
Member

Thanks :)

On Mon, Sep 29, 2014 at 1:28 PM, David Gil [email protected] wrote:

I didn't know that chardet isn't a default module although is installed in
all my systems without doing anything. Sorry about that.

In that case @naparuba https://github.com/naparuba, I should have check
if chardet is installed before using it. Checkout the following patch:
dgilm/mod-import-mysql@a36ea5a
dgilm@a36ea5a

I can prepare a new pull request if you want.


Reply to this email directly or view it on GitHub
#7 (comment)
.

dgilm added a commit to dgilm/mod-import-mysql that referenced this pull request Sep 29, 2014
The use of chardet python library is quite useful in order to detect character encodings, but it's not installed in all systems by default. So, it's a good practice to check its availability before using it.

This pull request enhances shinken-monitoring#7
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.

4 participants