Add gsheet #163
Add gsheet #163
Conversation
| @@ -23,6 +23,7 @@ Custom_scripts/ | |||
| Data_Files/MicroscopyCalibration/Files/ | |||
| .pytest_cache/ | |||
| .python-version | |||
| wranglertools/.config/gspread/authorized_user.json | |||
There was a problem hiding this comment.
If there is nothing worth tracking in .config I would gitignore the whole thing just to be safe... You also may want to provide another means to specify these credentials since folks who install from PyPi won't have this path.
tests/test_import_data.py
Outdated
| assert message0 == outlist[0] | ||
| assert message1 == outlist[1] | ||
| assert message2 == outlist[2] | ||
| # def test_workbook_reader_post_ftp_file_upload(capsys, mocker, connection_mock, workbooks): |
There was a problem hiding this comment.
I would consider just adding a comment here indicating the ftp support has been removed.
wranglertools/import_data.py
Outdated
| 'image/tiff', | ||
| ) | ||
|
|
||
| # If modifying these scopes, delete the file token.json. |
There was a problem hiding this comment.
Where would this be located if installing from PyPi? Make targets fixing these things may be useful... You may also want to read the SCOPES from a config file.
wranglertools/import_data.py
Outdated
| def google_authenticate(): | ||
| gsauth = None | ||
| creddir = pp.Path(__file__).parent.joinpath('.config', 'gspread') | ||
| gsauth = gspread.oauth( | ||
| credentials_filename=creddir.joinpath('credentials.json'), | ||
| authorized_user_filename=creddir.joinpath('authorized_user.json'), | ||
| scopes=SCOPES | ||
| ) | ||
| return gsauth |
There was a problem hiding this comment.
Will this work for users who pip install the package? You probably want to allow folks to set a path in an environment variable as well and maybe fallback to this if not found? I would also definitely add everything in .config to .gitignore.
| @@ -1,6 +1,6 @@ | |||
| [tool.poetry] | |||
| name = "Submit4DN" | |||
| version = "3.1.1" | |||
| version = "3.2.0" | |||
There was a problem hiding this comment.
You might consider bumping major version since you are removing FTP support.
wranglertools/import_data.py
Outdated
| """Set of check for connection, file, dryrun, and prompt.""" | ||
| def check_and_return_input_type(inputname): | ||
| if pp.Path(inputname).is_file(): | ||
| xlsx_mime = 'application/vnd.openxmlformats-officedocument.spreadsheetml.sheet' |
There was a problem hiding this comment.
This is probably a good candidate for a constant, as we've seen issues with this mime in the past.
wranglertools/import_data.py
Outdated
| print("ERROR: URL provided does not appear to be google sheet url") | ||
| sys.exit(1) | ||
| # parse out the bookId | ||
| inputname = re.search("/d/([A-Za-z0-9_-]+)/*", inputname).group(1) |
There was a problem hiding this comment.
I like constants for regex as well as they are often too complex to interpret inline, defining at top level and giving it a good name with an example or two is most helpful.
wranglertools/import_data.py
Outdated
| inputname = re.search("/d/([A-Za-z0-9_-]+)/*", inputname).group(1) | ||
| if not re.match("^[A-Za-z0-9_-]+$", inputname): | ||
| print("ERROR: invalid format of the google sheet ID in input - {}".format(inputname)) | ||
| return inputname, 'gsheet' |
There was a problem hiding this comment.
gsheet and excel are probably good candidates for constant.
wranglertools/import_data.py
Outdated
| print(f"ERROR: File {inputname} not recognized as excel file") | ||
| sys.exit(1) | ||
| return inputname, 'excel' | ||
| elif inputname.startswith('http'): |
There was a problem hiding this comment.
You probably want to enforce https here.
wranglertools/import_data.py
Outdated
| if booktype == 'gsheet': | ||
| gauth = google_authenticate() |
There was a problem hiding this comment.
As mentioned before it's highly probable you will want to provide alternative means of specifying the Google credentials, either by argument directly or environment variable to path. I don't see how a user installing from pip could easily use this. You'll need at least some barebones documentation on this as well.
…still need to fix a few tests
DRAFT - Do not merge
My main question remaining about this is what would be the best practice regarding the credentials.json file required for this to work (the one that are not really secrets as indicated above).
Options:
Additional changes in the PR include removing any support for downloading or uploading via ftp.