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

More clean up and dependency updates #155

Merged
merged 6 commits into from
Mar 29, 2022
Merged

More clean up and dependency updates #155

merged 6 commits into from
Mar 29, 2022

Conversation

aschroed
Copy link
Member

@aschroed aschroed commented Mar 17, 2022

This PR builds off the long standing but recently merged PR of the same name that removed python2 support and got things working in both Python 3.6(.1 and above) and 3.7.

Dependencies were updated and tested on Python 3.8 and 3.9 so can use same versions as dcicutils. Looks like deprecated system time library dependencies are used in xlrd and xlwt (that are really only for older xls file versions in any case) so will likely need to update these packages which is not trivial. Will look into it when I have time

Some unneeded (leftover) dependencies were removed.
NOTE: there used to be an awscli dependency likely to support the subprocess('aws s3 cp') call but I built several virtual environments and installs without this dependency and everything still worked fine -but- I wasn't sure if this was because awscli or similar functionality came in with dcicutils or if the dependency is still needed and for some reason the subprocess call is able to use my non-virtual env system installed as tools and not sure how to test.

Some old and unused imports - some of which I believe were related to python2 support - were removed.

At some point a python-eggs folder was included in the repo in the wranglerstools directory. This likely should never have been in the repo but is not used or needed with the current poetry packaging.

…ved the egg-info folder that was incorporated at some point in the past - maybe by mistake
@coveralls
Copy link

coveralls commented Mar 17, 2022

Coverage Status

Coverage decreased (-0.04%) to 85.485% when pulling b36107f on upd_py_dep into 984e26d on master.

@aschroed aschroed changed the title More clean up and dependency updates DRAFT: More clean up and dependency updates Mar 17, 2022
@aschroed aschroed changed the title DRAFT: More clean up and dependency updates More clean up and dependency updates Mar 18, 2022
@aschroed
Copy link
Member Author

Updating xlrd to xlrd3 allows this to build on python 3.8 and 3.9 and this seems a reasonable approach for now as it allows support for reading both xls and xlsx workbooks.
However, it should be assessed whether migrating to openpyxl and/or supporting google sheets might be in order in the future.

@@ -343,9 +343,9 @@ def test_get_uploadable_fields_mock(connection_mock, mocker, returned_vendor_sch
def xls_to_list(xls_file, sheet):
"""To compare xls files to reference ones, return a sorted list of content."""
from operator import itemgetter
import xlrd
import xlrd3
Copy link
Member

Choose a reason for hiding this comment

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

I'd move this to a top level import. Same comment below.

@aschroed aschroed merged commit 70d017d into master Mar 29, 2022
@aschroed aschroed deleted the upd_py_dep branch March 29, 2022 15:45
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