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

feat: Add config to enable instant file download #68

Closed

Conversation

artem-vavilov
Copy link
Contributor

Issue: #66

@andrii-bodnar
Copy link
Member

Hi @artem-vavilov, thanks for the contribution!

This download file functionality doesn't fit well into this API client, and it's something that's unique to this client. None of our other API clients have it, so I've even thought about removing it.

I'm not sure if it's a good idea to introduce the new config parameter, so I'd suggest changing the behavior a little bit.

Let's download the file only in cases when the destination parameter is specified. In that case, we'll still return the path to the downloaded file.

If the destination parameter is not passed - return the full API response.

For example, with the destination:

crowdin.download_file(file_id: your_file_id, destination: your_destination, project_id: your_project_id)

# -> should return the downloaded file path.

Without the destination:

crowdin.download_file(file_id: your_file_id, project_id: your_project_id)

# -> should return an object from the response

# {
#   "url": "https://production-enterprise-importer.downloads.crowdin.com/992000002/2/14.xliff?......",
#   "expireIn": "2019-09-20T10:31:21+00:00",
#   "etag": "ebd69a1b7e4c23e6d17891a491c94f832e0c82e4692dedb35a6cd1e624b62"
# }

Please let me know what you think about this!

@artem-vavilov
Copy link
Contributor Author

If the destination parameter is not passed - return the full API response.

@andrii-bodnar I also considered this option but rejected it because it would dramatically change the behavior of some methods.

However, if you don't mind, I'm happy to implement this!

@andrii-bodnar
Copy link
Member

andrii-bodnar commented Feb 2, 2024

@artem-vavilov yes, it could be a bit risky. We'll try to release it by adding a warning message to release notes about this change.

Could you please take a look at the #67 issue as well?

It would be great if we could address both of these issues within a single release.

@artem-vavilov
Copy link
Contributor Author

New PR: #69

@artem-vavilov artem-vavilov deleted the feat/add-download-option branch February 3, 2024 15:26
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