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.

Sign up for GitHub

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

Jump to bottom

Update urllib.request to be the preferred urllib #428

Merged
merged 2 commits into from
Mar 15, 2021
Merged

Update urllib.request to be the preferred urllib #428

merged 2 commits into from
Mar 15, 2021

Conversation

dcslagel
Copy link
Collaborator

Description

This pull-request attempts to resolve the problem described in issue #426 "Have an issue with urllib2 when using the lasio package". It changes the import order for urllib.request and urllib2 so that urllib.request is tried first. urllib.request is recommended for python3, urllib2 is no-longer updated for python3.

I left urllib2 in as a fallback for python2 support even though Lasio no-longer supports python2. Let me know if it should be removed now and I'll make the update to this pull-request.

Test Notes:

The test results are the same before and after the code change:

Name                       Stmts   Miss  Cover
----------------------------------------------
lasio/__init__.py             13      2    85%
lasio/convert_version.py      20     20     0%
lasio/defaults.py             11      0   100%
lasio/examples.py             42     10    76%
lasio/excel.py                88     34    61%
lasio/exceptions.py            6      0   100%
lasio/las.py                 407     59    86%
lasio/las_items.py           199     31    84%
lasio/las_version.py          50     14    72%
lasio/reader.py              406     32    92%
lasio/writer.py              175     10    94%
----------------------------------------------
TOTAL                       1417    212    85%

Let me know if this change could be accepted (or rejected) or
needs some additional changes to be approved and merged.

Thank you,
DC

@kinverarity1
Copy link
Owner

kinverarity1 commented Mar 10, 2021 via email

@dcslagel
Copy link
Collaborator Author

dcslagel commented Mar 10, 2021 •
edited
Loading

Okay, Per @kinverarity1's response, I've removed urllib2 python2 support. In addition, import urllib is moved out of the try to the file level. Also the try is removed since we are no longer expecting an import error on urllib.

Test coverage for reader.py changes to less statements and higher coverage because the urllib2 section is removed:

Name                       Stmts   Miss  Cover
----------------------------------------------
...
lasio/reader.py              396     25    94%
----------------------------------------------
TOTAL                       1407    205    85%

Let me know if this change could be accepted (or rejected) or
needs some additional changes to be approved and merged.

Thank you,
DC

@kinverarity1
Copy link
Owner

Thank you

@kinverarity1 kinverarity1 merged commit 2b80f85 into kinverarity1:master Mar 15, 2021
@dcslagel dcslagel deleted the update-urllib-import branch March 15, 2021 16:47
@dcslagel dcslagel mentioned this pull request Mar 15, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@dcslagel @kinverarity1