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

Fix: Log all output even binaries #663

Merged
merged 2 commits into from
Jun 19, 2017
Merged

Conversation

stoivo
Copy link
Member

@stoivo stoivo commented May 31, 2017

Some command in inline diff are workning with binaries so
when we try to log. Try to decode it as utf-8, otherwise
fail.

closes #662

@stoivo
Copy link
Member Author

stoivo commented Jun 12, 2017

A review?

@randy3k
Copy link
Collaborator

randy3k commented Jun 16, 2017

I am not sure what was the issue and why it fixes the issue....perhaps @divmain may know more.

@stoivo
Copy link
Member Author

stoivo commented Jun 16, 2017

In this PR #606 we do some command without decoding the output because we are sending it back in some other command. I made it pass the binary around therefor I would need to deal with the encoding and it would always keep the correct encoding.

And I make a bug. When you would log this command which only worked with binaries it would save the messages. Here https://github.com/stoivo/GitSavvy/blob/409e5eefbf39f53c31d3d6253a6ea5246e7bc583/common/util/debug.py#L36 when we dump it to json if fails since json.dump don't work with binaries.

@@ -7,6 +7,7 @@

_log = []
enabled = False
encoding_not_utf8 = "{} was sent as binaries and we dont know the encoding, not utf-8"
Copy link
Collaborator

Choose a reason for hiding this comment

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

I guess it is a convention to use upper cases for such constant!?

Copy link
Member Author

Choose a reason for hiding this comment

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

Does python make it immutable if it is uppercase?

Copy link
Member

@asfaltboy asfaltboy Jun 16, 2017

Choose a reason for hiding this comment

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

Strings are always immutable, and names can always be overridden, no matter the casing. It's only a convention for readability sake (AFAIK).

Copy link
Member Author

Choose a reason for hiding this comment

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

That's different than ruby, which I have more experience with.

Some command in inline diff are workning with binaries so
when we try to log. Try to decode it as utf-8, otherwise
fail.

closes timbrel#662
@stoivo
Copy link
Member Author

stoivo commented Jun 19, 2017

Merge?

@randy3k randy3k merged commit c2631c5 into timbrel:master Jun 19, 2017
@stoivo stoivo deleted the log_encoding branch June 20, 2017 06:12
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.

GitSavvy: view recorded log throws an exception
3 participants