Fix Python 3 TypeError: sequence item 0: expected str instance, bytes found on call to self.debuglog #128
Fix Python 3 TypeError: sequence item 0: expected str instance, bytes found on call to self.debuglog #128matteius wants to merge 7 commits intolinsomniac:masterfrom matteius:master
Conversation
expected is a list of bytestrings which cannot be joined with the string ' or ' so the fix is to make that string a bytestring anyway since the expected argument is always an array of bytestrings.
|
I am going to re-open this PR now that Python 3.3 was removed from the Travis CI. |
|
Trying to re-open PR |
|
I guess python 3.3 support hasn't been removed from this project yet after-all. |
|
Could you add a regression test? |
|
@timgraham Started working on that per your request, problem is it seems the existing test stack seems to depend on memcached running on localhost, because when I turn it off I get 19 test failures, However when I turn on memcached VERSION 1.4.33 Ubuntu I still get a single test failure in the doc string tests (which are my least favorite kind of tests) due to perhaps the server is behaving differently on the set_multi with a key_prefix test returns a different format than expected except I can't figure out how to print what the difference is. Also the docs seem to indicate that this isn't the case, from README.md:
I'm not sure I understand what the patch is exactly. I started down the road of patching the tests to work with mockcache https://pypi.python.org/pypi/mockcache though I wasn't sure how the library maintainers view whether their test suite is unit tests or integration tests. Right now it feels clunky to depend on an older version of the memcached server, though I am not sure what version does allow this nose test to pass:
Additionally, as far as adding a regression test goes, I cannot really test the scenario the above code change fixes if the test is going to run against an actual functioning memcached server -- it would have to be a test with a mock that causes something in the server's response to mismatch the expected values, which normally only happens when you have a misbehaving node. |
|
I get the same error in the doc test. Looks like they don't run on Travis. I've fixed both issues in #132. I guess some sort of mocking may be needed to test this. |
|
This change is also made in #108 and there's a test there that fails with this reverted so maybe you can borrow that. |
|
Thanks that was helpful. I ended up writing my own test, but I also discovered now that nothing is running those tests. VirtualBox:~/python-memcached$ nosetests Ran 0 tests in 0.001s OK |
|
Ok should be all set now! I'd note a few comments on possible future enhancements: |
timgraham
left a comment
timgraham
left a comment
There was a problem hiding this comment.
You can squash commits when updating, thanks!
| /python_memcached.egg-info | ||
| .tox | ||
| .coverage | ||
| .idea |
There was a problem hiding this comment.
Please revert these changes.
There was a problem hiding this comment.
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.
In my experiencing maintaining Django, we've recommended to use git config core.excludesfile for 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 at git 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.
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.
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.
There was a problem hiding this comment.
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:
Patterns which a user wants git to ignore in all situations (e.g., backup or temporary files
generated by the user’s editor of choice) generally go into a file specified by
core.excludesfile in the user’s ~/.gitconfig
Quoted from: https://git-scm.com/docs/gitignore/1.7.12
There was a problem hiding this comment.
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.
|
|
||
| import mock | ||
| import six | ||
| import unittest |
There was a problem hiding this comment.
This import shouldn't be resorted. Standard library imports are separated from third-party imports.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Sorry, I didn't know that.
There was a problem hiding this comment.
Yeah, I only included it in the test-requirements.txt for users of python 2.7
| 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' |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
note I ran this using pytest Remove pytest file flake8 the great flake8 bites again
|
Ok I squashed the commits down so that no one will be able to tell it was my preference to use pytest until I saw it break Travis CI and turned it back to nose (for which I turn my nose up to). |
|
@matteius: I appreciate your work on this, but I do not appreciate how you have been treating @timgraham. I would like you to contemplate what you have said here, and you probably should also read the github TOS, in particular section C items 2 and 3, before participating in this or other projects. https://help.github.com/articles/github-terms-of-service/#c-acceptable-use |
|
@linsomniac I don't appreciate your role in this library, or how you belittle the contributions of other selectively. That is why myself and others have already converted off of this library. I hope you reflect on how you have wasted my time by not granting me contributor status. Furthermore I have two more points to make about the .gitignroe file besides the fact that its silly you would want to allow any user to possibly check in an IDE configuration directory. My second point would be the Django pypi packages and yours does in-fact include .gitignore file in the pypi package. But the Django project doesn't allow people to exclude IDE directories from Django by using the .gitignore file, which I was unaware of. That is their decision, and not one I agree with. However I will also point out that djangorestframework ignores .* so all hidden folders and that would also solve this problem. That's just it, you don't want to listen about the problems, you want your way or the highway. As to you thinking I was attacking timgraham in some-awful way, I think you are mistaken, or at least don't understand how I approach people on the internet that I don't know. The fact that you are making social issues more relevant than the proper functioning of your own library is your business, but I am tired of the recuring friction to contribute something in this community when things are broken. I was happy when I found the djangorestframework community made it more possible for me to become a contributor. I think you calling out the C 2 and 3 section of GitHub TOS is a bit extreme and unwarranted. I would say that is more harassing to me and my efforts as I ever was to you Tim's. I'm incredibly angry with you, and I will no longer be helping maintain your project. I will consider deleting my fork as well since there is no value here for you. |
|
Please do quote the exact part of the TOS that you think I am infringing upon, and do take it up with GitHub. |
|
@matteius: I have quoted exactly the parts of the TOS I believe you are violating and I have already taken it up with GitHub. As far as making @timgraham a contributor and not you: he has offered to help on this and other issues repeatedly over more than a year, and additionally he has been literally the ONLY PERSON, yourself included, who has offered to help in a contributor role. Your behavior in this PR makes me quite glad to have never given you a contributor bit. Of course, I had never really considered it because you had never offered to. It is clear you are angry, and I understand that. Your taking your anger out on @timgraham is unacceptable, unwarranted, and undeserved. He was trying to help you get this PR accepted, and started berating him. This is not an acceptable way to contribute to a project. I offered to maintain this project when it was abandoned by the previous maintainer, and worked on it for years. Recently, I have not had time to put extensive work on it. So unless PRs were obvious or had community discussion on them that showed a clear consensus, I couldn't integrate them. Tim has, quite literally, been the only person to offer to help. Your abuse of him is unappreciated. |
|
I was wrong @linsomniac because you are the real asshole here. I definitely didn't deserve the abuse I have gotten from you for my decision to try and help. You and I are not on good terms, and I hope that GitHub isn't some authoritarian service that is going to come after me exclusively. You and I are done, I will advocate that people use another library. |
|
Calling you out for abusing Tim is not abuse. |
|
For reference to anyone coming into this in the future, note that matteius has edited many of his comments to remove his most abusive comments to timgraham. You may also notice I edited my first comment in this thread, I edited it immediately after posting to fix the formatting of the quoted block. I'm posting this comment as matteius's revision make me look like I've overreacted. |
|
Updated in #134. |
4 participants
expected is a list of bytestrings which cannot be joined with the string ' or ' so the fix is to make that string a bytestring anyway since the expected argument is always an array of bytestrings.
The original issue is that memcached responds in an unexpected way and then the application blows up on the debuglog line with: TypeError: sequence item 0: expected str instance, bytes found.