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

Python3 + Linted + Switched to requests lib #2

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

Conversation

justinaustin
Copy link

Made the following changes:

  • Converted code to python3
  • Linted code with flake8
  • Switched to the requests lib from urllib and urllib2

@sanderjo
Copy link
Owner

sanderjo commented Jul 23, 2017

Hi Justin,

Impressive. Nice to know someone else uses the code. My remarks:

Incorrect speed

Your code IMHO gives incorrect results:

sander@Stream-13:~/git/python3-fastcom$ python3 fast_com_example_usage.py
Start speedtest against fast.com ...
Result: 144.6 Mbps
... Done
sander@Stream-13:~/git/python3-fastcom$ python3 fast_com_example_usage.py
Start speedtest against fast.com ...
Result: 147.3 Mbps
... Done

145 Mbps is impossible on my VDSL-connection. The site https://fast.com/ and my python2 tool report around 35 Mbps.
So some kind of bug?

Python2 compatibility

I would like to keep the code python2 compatible, because I want it to be stay compatible with sabnzbd and NAS-devices which do have python2. After installing python-requests, this is the result of running with python2:

sander@Stream-13:~/git/python3-fastcom$ python2 fast_com_example_usage.py
Start speedtest against fast.com ...
('Result:', 141.0, 'Mbps')
... Done
Exception in thread Thread-2 (most likely raised during interpreter shutdown):Exception in thread Thread-3 (most likely raised during interpreter shutdown):
Traceback (most recent call last):
  File "/usr/lib/python2.7/threading.py", line 801, in __bootstrap_inner
Traceback (most recent call last):
  File "/usr/lib/python2.7/threading.py", line 801, in __bootstrap_inner

Do you think that is solvable?

@justinaustin
Copy link
Author

Hey,

I'll check to see about the speed bug. I'll also check about keeping python3 compatibility but I am less confident in that.

Thanks!

Copy link

@adeak adeak left a comment

Choose a reason for hiding this comment

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

A few notes on the bug in the implementation and some suggestions about python 2-3 compatibility.

mbps = "%.1f" % net_bits
print("Loop", loop, "Total MB", total_mb, "Delta_MB", delta_mb)
print("Speed kB/s:", speedkBps, "aka Mbps ", mbps)
lasttotal = total
Copy link

Choose a reason for hiding this comment

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

This is the source of the bug which leads to a spurious (high) result. This line should be outside the if verbose: block, semantically speaking I'd put it at the end of the loop, just before time.sleep().

time.sleep(sleepseconds)

Mbps = (application_bytes_to_networkbits(highestspeedkBps)/1024)
Mbps = float("%.1f" % Mbps)
Copy link

Choose a reason for hiding this comment

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

I know this is in the original source code, but while we're here I'd change this to round(Mbps,1).


'''
Python CLI-tool (without need for a GUI) to measure Internet speed with fast.com

Python CLI-tool to measure Internet speed with fast.com
'''


Copy link

Choose a reason for hiding this comment

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

I suggest adding from __future__ import print_function, division in order to get consistent results across python 2 and 3.

@@ -1,8 +1,7 @@
#!/usr/bin/env python
#!/usr/bin/env python3

Copy link

Choose a reason for hiding this comment

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

I suggest adding from __future__ import print_function here too for full 2-3 compatibility.

@adeak
Copy link

adeak commented Mar 30, 2018

I found the bug and posted some suggestions for python 2-3 compatibility. With the above future imports the output should be the same both in python 2 and 3. @sanderjo I can't reproduce the exception you get on 2.7, for me the above runs fine on both 3.6 and 2.7.

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.

3 participants