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

Various improvements re character encoding and testing #11

Open
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

asmundg
Copy link

@asmundg asmundg commented Nov 23, 2013

These are mostly improvements I needed myself, but they're relevant for #7, #9 and #10.

Åsmund Grammeltvedt added 8 commits November 8, 2013 14:23
As a side-effect, this makes us use unicode, which will be useful for
non-ascii characters in aythor and story names down the line.
Just to prove that my initial changes work.
Python doesn't let us know the terminal encoding if we're piping (which
may or may not be correct, depending on who you ask). So we need some
extra handling.

Incidentally, handling the output differently also makes is easier to
test that we're getting the right output.

I've only implemented this for show_stories thus far.
Fetching and displaying information are two distinct tasks, one of which
is functional.
Also basic testing and separation of concerns.
Seeing these is useful in some workflows,
@weldyss
Copy link

weldyss commented Nov 30, 2013

I've tested here and the story title stills bad. I received the unicode error on pivotal_tools show stories here

@jtushman
Copy link
Owner

jtushman commented Dec 4, 2013

yay -- pull request -- I will review shortly

@jtushman
Copy link
Owner

jtushman commented Dec 4, 2013

(and sorry I did not see this earlier)

ropez and others added 2 commits May 6, 2014 15:22
Apparently, ElementTree likes to do it's own decoding. It raises a
UnicodeError when given the decoded response.text, but it works fine
when given the raw response.content.
Use the binary response content instead of unicode
@cj13579
Copy link

cj13579 commented Feb 1, 2017

So I know that this pull hasn't been merged or accepted yet but I just downloaded the fork and it makes something that doesn't work, work! You should probably merge this 😄 👍

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.

5 participants