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

Python 3 support fix on _send method #71

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

Conversation

PabloLefort
Copy link

Running a Django v1.10 app with Python v3.4.3, the _send method breaks.
Stack trace:

  File "/home/pablo/.virtualenvs/test/local/lib/python3.4/site-packages/graphitesend/graphitesend.py", line 291, in _send
    self.socket.sendall(message.encode("ascii"))
AttributeError: 'bytes' object has no attribute 'encode'

...
graphitesend.graphitesend.GraphiteSendException: Unknown error while trying to send data down socket to ('172.17.0.2', 8000), error: 'bytes' object has no attribute 'encode'

@PabloLefort
Copy link
Author

@daniellawrence travis errors are not from the commit i made, can you give me a hand? Thanks in advance!

@coveralls
Copy link

coveralls commented Jan 6, 2017

Coverage Status

Coverage decreased (-0.3%) to 93.08% when pulling 634ba68 on PabloLefort:master into 6f84c56 on daniellawrence:master.

@Shir0kamii
Copy link
Collaborator

@PabloLefort I fixed the issue you were facing with the tests. However, the coverage decrease with your commit so could you please add a test to make it at least the same as now.

Also, please note that an other ongoing PR (#70) is trying to fix a Python 3 issue and I think it's the same issue. Can you please explain what was the issue you were facing ? Our Python 3 test suite is passing so I don't understand what could be the error.

I like the fact that the PR #70 is regardless of the python version. But I also like the simplicity of your PR. You could still make it simpler by using an and clause rather than two if statements.

@PabloLefort
Copy link
Author

@Shir0kamii thanks for the tests! Yes, i can make a test for the coverage.
I see the PR #70 but as you say, i also think this is a more simple fix.

The problem was that sending a test metric graphite_client.send('testing', 10) throws the stack trace of above.

Your suggest is even more simpler! I'll update it.

@coveralls
Copy link

coveralls commented Jan 7, 2017

Coverage Status

Coverage decreased (-0.3%) to 93.127% when pulling ffb0321 on PabloLefort:master into d18b986 on daniellawrence:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.3%) to 93.103% when pulling 961cfc6 on PabloLefort:master into d18b986 on daniellawrence:master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.3%) to 93.103% when pulling 961cfc6 on PabloLefort:master into d18b986 on daniellawrence:master.

@Shir0kamii
Copy link
Collaborator

@PabloLefort Agreed! Once you make the coverage stage pass, I'll merge your pull request.

@PabloLefort
Copy link
Author

@Shir0kamii It could be something like this?

    def test_send_values_as_bytes(self):
        graphite_instance = graphitesend.init()
        metric = 'metric'
        if sys.version_info >= (3, 0) and type(metric) is bytes:
            metric = str(metric, 'utf-8')
            response = graphite_instance.send(metric, "1", "1")
            response = str(response)
            self.assertEqual('metric 1.000000 1' in response, True)

Thank you!

@Shir0kamii
Copy link
Collaborator

@PabloLefort I'm not sure it would increase the coverage.

Coverage is just a tool verifying that every line of code is executed. So to increase coverage you will need to make sure the test you write execute the lines of code you added. It means that you will have to send a byte object.

Also because your code will only ever be executed on Python 3, you won't be able to increase Python 2's coverage. But that's okay, I'll merge it if Python 3 pass coverage test.

@PabloLefort
Copy link
Author

@Shir0kamii Thanks! I'll update the test tonight.

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.

None yet

3 participants