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

Properly handle unicode data. #47

Merged
merged 2 commits into from Jan 10, 2017

Conversation

Projects
None yet
3 participants
@mpena2099
Contributor

mpena2099 commented Dec 11, 2016

Same kind of error as grantmcconnaughey#3


@coveralls

This comment has been minimized.

coveralls commented Dec 11, 2016

Coverage Status

Coverage decreased (-0.1%) to 93.006% when pulling a943eff on mpena2099:master into f14e75c on chartit:master.

@atodorov

This comment has been minimized.

Contributor

atodorov commented Dec 11, 2016

@mpena2099 thanks for the PR. Please see commit 8dac0f7. You need to add the same version handling.

Also can you add a new test which demonstrates the bug & verifies the fix? Thanks!

@mpena2099

This comment has been minimized.

Contributor

mpena2099 commented Dec 12, 2016

OK @atodorov, I'll try again soon.

Thanks,
Mauricio de O. Pena

@coveralls

This comment has been minimized.

coveralls commented Dec 16, 2016

Coverage Status

Coverage decreased (-0.03%) to 93.103% when pulling 4a73016 on mpena2099:master into f14e75c on chartit:master.

@mpena2099

This comment has been minimized.

Contributor

mpena2099 commented Dec 16, 2016

@atodorov, still some check errors...

@atodorov

This comment has been minimized.

Contributor

atodorov commented Dec 18, 2016

@mpena2099 sorry to bitch about it but you will have to add a few more bits to this PR before I can merge it:

  • Update changelog in README
  • Add a new test (or document which existing test covers your patch)
  • Squash all commits together

Please see commit 8dac0f7 for reference. There you will also find some unicode data, which is used during testing. Feel free to add more data if the existing one doesn't fit the charts you want to create.
NOTE: views in demoproject/ are also used for testing.

EDIT: don't worry about the failing TravisCI jobs, there are lots of pylint errors currently but please make sure code you touch doesn't introduce more pylint errors.

@atodorov atodorov merged commit 2c0da34 into chartit:master Jan 10, 2017

0 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
coverage/coveralls Coverage decreased (-0.03%) to 93.103%
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment