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

refactor: fal file #16

Closed
wants to merge 10 commits into from
Closed

refactor: fal file #16

wants to merge 10 commits into from

Conversation

badayvedat
Copy link
Contributor

No description provided.

@badayvedat badayvedat force-pushed the refactor-fal-toolkit-file branch from a0c81f4 to 4017985 Compare January 5, 2024 13:41
mederka
mederka previously approved these changes Jan 19, 2024
Copy link
Contributor

@mederka mederka left a comment

Choose a reason for hiding this comment

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

Great job, @badayvedat 👏

As discussed. I think that maybe the class name File is a bit misleading because in reality it's not referring to a file but to a link to a file. It works magically when it's used inside a fal runtime, but not so in users local environment. I like your idea of potentially renaming this class to FileUrl or FileLink.

projects/fal/src/fal/toolkit/__init__.py Show resolved Hide resolved
Comment on lines +59 to +60
with data.data as file:
gcp_blob.upload_from_file(file, content_type=data.content_type)
Copy link
Contributor

Choose a reason for hiding this comment

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

Will this work if filename == "" ?

Copy link
Contributor Author

@badayvedat badayvedat Jan 23, 2024

Choose a reason for hiding this comment

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

good catch! we should've raised an error in that case

mederka
mederka previously approved these changes Jan 22, 2024
Copy link
Contributor

@mederka mederka left a comment

Choose a reason for hiding this comment

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

I say, once the tests pass, we ship it!

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.

2 participants