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

issue/97 accept path like objects for file uploads #463

Conversation

theunkn0wn1
Copy link
Contributor

Proposed Changes

Add canvas.upload.FileOrPathLike type, which represents any os.PathLike, str, or io based file like and path like objects.
Add support in canvas.upload.Upload to accept any FileOrPathLike, as per #97 /
Add type annotations to all consumers of file arguments which are forwarded into canvas.upload.Upload.

Added two tests that use pathlib.Path objects to assert correctness.

Fixes #97 .

@theunkn0wn1
Copy link
Contributor Author

Ok. strangely this works in the test suite but not in my downstream (fails in an actual upload). It appears the tests I added were not sufficient. I recommend not merging this yet til I figure out how to properly test this & fix the bug.

@theunkn0wn1
Copy link
Contributor Author

image
works in downstream now.
It appears the tests were accurate, and i had a stale version of this project in the test environment leading to my last message 🤦

@Thetwam Thetwam added this to the CanvasAPI v2.2.0 milestone Feb 22, 2021
@coveralls
Copy link

coveralls commented Mar 23, 2021

Coverage Status

Coverage remained the same at 100.0% when pulling fb11600 on theunkn0wn1:issue/97-accept-path-like-objects-for-file-uploads into 971ce43 on ucfopen:develop.

self.kwargs["name"] = os.path.basename(file.name)
self.kwargs["size"] = os.fstat(file.fileno()).st_size
if isinstance(file, Path):
self.kwargs.update({"name": file.name, "size": file.stat().st_size})
Copy link
Member

Choose a reason for hiding this comment

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

From what I can see, I think this code is unreachable with our current public API. We open the file before we call get_upload_token, so it's never a Path.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I believe that's correct, this change can likely be reverted.
IIRC I added this check while debugging something and didn't remove it after.

@jessemcbride
Copy link
Member

jessemcbride commented Mar 23, 2021

So, the good (and probably obvious) news: The changes work! Still able to upload files (paths and file handlers alike).

This is almost ready to go, but I have a question re: coverage. Our coverage drops here because of the isinstance(file, Path) case, but I think we can just remove the condition based on the current public API. @theunkn0wn1 Does that make sense, or am I missing something?

@theunkn0wn1
Copy link
Contributor Author

Sounds logical to me. pushed a commit to address that point.

Copy link
Member

@Thetwam Thetwam left a comment

Choose a reason for hiding this comment

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

Thanks for your work on this! LGTM 👍

@Thetwam Thetwam merged commit 81f285f into ucfopen:develop Mar 24, 2021
@theunkn0wn1 theunkn0wn1 deleted the issue/97-accept-path-like-objects-for-file-uploads branch March 25, 2021 01:29
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.

Accept path-like objects for file uploads
4 participants