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

Initial unicode support for Text and TextPos #112

Merged
merged 1 commit into from
Apr 27, 2017

Conversation

cyberscribe
Copy link
Contributor

See line 38 of papirus/textpos.py - I am not sure why we are using decode('string_escape') but it is not utf-8 friendly. All tests seem to work fine without it. Please advise before merging.

@shawaj
Copy link
Member

shawaj commented Apr 22, 2017

@cyberscribe - thanks very much. Not got a PaPiRus with me at home this weekend but will test and merge next week with @francesco-vannini

@cyberscribe
Copy link
Contributor Author

Excellent, I tested what I could but would be great to have another pair of eyes. Also do be sure to check out the function I dropped--didn't affect all the sample cases I tried, but I'm not sure if it was put in there for an edge case I might have missed. Which reminds me--are there any unit tests for this package?

@shawaj
Copy link
Member

shawaj commented Apr 22, 2017

When you say unit tests, what do you mean? Sorry we're not really software experts so bear with us on the terminologies :-)

And which function did you drop?

@cyberscribe
Copy link
Contributor Author

Hi @shawaj -- I dropped the .decode('string_escape') from line 38.

Here's a decent introduction to unit testing: https://python-guide-pt-br.readthedocs.io/en/latest/writing/tests/

It would basically save time to have a battery of all edge cases to test, because future contributors could validate that their changes have not broken anything by running these automated tests before submitting a pull request.

@shawaj
Copy link
Member

shawaj commented Apr 22, 2017

Awesome. That sounds great. Could you submit an issue for that unit testing? Sounds like a very good idea! Would certainly save us a lot of time!

@cyberscribe
Copy link
Contributor Author

cyberscribe commented Apr 26, 2017 via email

@francesco-vannini francesco-vannini merged commit f955fc9 into PiSupply:master Apr 27, 2017
@francesco-vannini
Copy link
Contributor

This solves #10. Thanks @cyberscribe for your help!
Tested on both text.py and textpos.py

@cyberscribe
Copy link
Contributor Author

Great! Happy to help, and happy to be able to draw unicode symbols on my PaPiRus!

@shawaj
Copy link
Member

shawaj commented Apr 27, 2017

@francesco-vannini did you check that the removal of decode('string_escape') hasn't caused any knock on errors?

Was thinking it might stop line breaks from being recognised...

@francesco-vannini
Copy link
Contributor

I tested that and it still works. I don't quite see why the decode method was used really
http://www.tutorialspoint.com/python/string_decode.htm
@BenScarb could you maybe clarify this?

@shawaj
Copy link
Member

shawaj commented Apr 27, 2017

Perhaps it was to stop attack code being introduced - although that would be a bit weird for you to attack your own PaPiRus

@cyberscribe
Copy link
Contributor Author

cyberscribe commented Apr 27, 2017

Seems something like this will strip non-printable Unicode characters from the output if there is still a concern about data sanitisation: http://stackoverflow.com/a/93557

@BenScarb
Copy link
Contributor

Can we pretend I did it for data security? ;)
I added it in as it was a way to make sure the /n and /r were recognised, without it (on my set up) throwing /n line breaks weren't being seen and were getting shown on the display.
I'll do a pull and see what happens, it'll probably work a treat.

If it's working without, great stuff, the less code in there, the faster it'll work!

@shawaj
Copy link
Member

shawaj commented Apr 27, 2017

@francesco-vannini did you test with both /n and /r line breaks?

If so then happy days!

@francesco-vannini
Copy link
Contributor

I have checked again with and without the changes in this PR. Using \n or \r still works (not to be confused though with /n and /r which are printed on the screen)

@francesco-vannini francesco-vannini mentioned this pull request May 11, 2017
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