-
Notifications
You must be signed in to change notification settings - Fork 198
Fix Python 3 TypeError: sequence item 0: expected str instance, bytes found on call to self.debuglog #128
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 Python 3 TypeError: sequence item 0: expected str instance, bytes found on call to self.debuglog #128
Changes from all commits
701bd6a
cea0d76
f634319
a39bc06
2a24a79
0fd1c32
a62722c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3,3 +3,5 @@ | |
| /python_memcached.egg-info | ||
| .tox | ||
| .coverage | ||
| .idea | ||
| lastfailed | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,3 +1,4 @@ | ||
| nose | ||
| coverage | ||
| hacking | ||
| mock |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,10 +1,10 @@ | ||
| from __future__ import print_function | ||
|
|
||
| import unittest | ||
|
|
||
| import mock | ||
| import six | ||
| import unittest | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This import shouldn't be resorted. Standard library imports are separated from third-party imports.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That's not true, locally it failed flake8 due to the imports needing to be alphabetical for lsort. Maybe you don't realize that mock is a part of the standard library in Py3?
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sorry, I didn't know that.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, I only included it in the test-requirements.txt for users of python 2.7 |
||
|
|
||
| from memcache import Client, SERVER_MAX_KEY_LENGTH, SERVER_MAX_VALUE_LENGTH # noqa: H301 | ||
| from memcache import Client, _Host, SERVER_MAX_KEY_LENGTH, SERVER_MAX_VALUE_LENGTH # noqa: H301 | ||
|
|
||
|
|
||
| class FooStruct(object): | ||
|
|
@@ -166,6 +166,13 @@ def test_disconnect_all_delete_multi(self): | |
| ret = self.mc.delete_multi({'keyhere': 'a', 'keythere': 'b'}) | ||
| self.assertEqual(ret, 1) | ||
|
|
||
| @mock.patch.object(_Host, 'readline') | ||
| def test_memcached_error_unexpected_reply(self, mock_readline): | ||
| """Test that the debug log executes properly in error response.""" | ||
| mock_readline.return_value = 'SET' | ||
| cache_key = ':cache:key:1' | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is there a special significance to this cache key name? Also, it looks like it doesn't need to be an intermediate variable. Can you add an assertion for the debug output?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These are stylistic decisions in writing the test. The test passes because the debug log doesn't blow up, and fails if you take out the change because of the python 3 TypeError. I'm done making your changes -- if you want to fix this library then just take my work and do it yourself because you are too damn picky. I've already converted our apps away from using this library, and was trying to make it better for people that still use it.
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm simply trying to improve the quality of the tests. Please don't assume malicious intent. I'll see if I can address my suggestions if you're not interested.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Well I personally like when there are intermediate variables in tests because it both names the value, and allows a debugger to introspect things. It also makes it easier later on if you want to then assert things about the key, because most likely if its not a named variable, another dev extending the test will copy the magic value all over the place. I've seen this time and time again.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I guess the problem with your suggestions is that its nit-picking a test that I choose to add. At least my test mocks out communication with Memcahed server to a certain extent. How about spending the time on the existing tests, mocking them, and adding more test coverage rather than be upset about an intermediate variable and a lack of assertion in the test case. The test runner still fails a test if something raises exception, so I feel like these are subjective stylistic preferences and not something enforced by flake8 or that actually improve the test. I wasn't willing to assert anything about the logging because I think its ridiculous it doesn't even use the logging module, but I think I've stated that already.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I just saw the passive aggressive thumbs up about adding an assertion of the awful logging methodology in your code base @linsomniac , but I am not going to do that. |
||
| self.mc.touch(cache_key) | ||
|
|
||
|
|
||
| if __name__ == '__main__': | ||
| unittest.main() | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please revert these changes.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No way. If you don't want that in the .gitignore then you don't need my time wasted on your pet project.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In my experiencing maintaining Django, we've recommended to use
git config core.excludesfilefor excluding IDE-specific files. That way if a new IDE comes along, its users can update their configuration own rather than requiring many projects to update.gitingore. I was also suggesting to exclude unrelated changes like this in a bug fix as that can be confusing when looking atgit blame.As an aside, this isn't my pet project, I just asked Sean this week if I could help him maintain it as it seems inactive.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well thats great that you have a Collaborator tag and I have nothing even though I've pointed you at several other issues in the past week that you have been actively helping clean up. I guess if thats the standard way to do it ... I wouldn't mind if the .gitignore got long because it would at least be a way to ignore things we know are common for the community. I personally use mercurial way more than git, so I don't know all the bells and whistles. It saddens me that the desired approach is that everyone does a one-off and pretends like the world is IDE agnostic.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm fine smashing it down to a single commit -- I mean personally I would never do it, because I am against re-writing history, but whatever. I really just want this bug fixed. I put 3 hours into this last night just trying to get it this far. So you figure I have 8 hours into trying to improve this library.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I really appreciate both of you collaborating on this and the effort both of you have put into this. It saddens me that this has devolved into anger. It would sadden me if you chose to not participate, especially after all your work, because ".idea" is not added to the project .gitignore. If that is the choice you want to make, I'll support your choice though.
Thank you @timgraham for the pointer about core.excludesfile, I was not aware of that. I agree that that is the right place for this exclude, after reading the git docs describing it as the preferred solution for files generated by a users editor. Quoting:
Quoted from: https://git-scm.com/docs/gitignore/1.7.12
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is because you are way more unreasonable than working with the DRF creator. I assume its because he is from Europe and you are not. The .idea directory will always be the PyCharm/IntelliJ local IDE config for all users, but I assume its because you want no reference to commercial software in your OSS. You would rather enforce these kind of standards than to actually realize that so many people are running into this problem with python 3 in their production code. Its not really acceptable when memcached error crashes the page for the end-user, but I guess we can argue about what should live in which .gitconfig. Either way, you'd have to be stupid to ever want users to add a .idea directory to the root of your project.