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

Add support for uploading multiple files #356

Merged
merged 8 commits into from
Mar 10, 2021
Merged

Add support for uploading multiple files #356

merged 8 commits into from
Mar 10, 2021

Conversation

Syfaro
Copy link
Member

@Syfaro Syfaro commented Jul 26, 2020

This allows for uploading multiple files for media groups or thumbnails. It also fixes a longstanding bug with using URLs.

There's a few breaking changes:

  • UploadFile has been renamed to UploadFiles, and now takes a []RequestFile instead of fieldname and file
  • All New*Upload and New*Share have been condensed into New*
  • All APIResponse and errors are returned as pointers

I'd love to get feedback on the API changes or what could be done to further improve things.

@HirbodBehnam
Copy link

HirbodBehnam commented Jul 26, 2020

Hello
Is using panic in the upload goroutine a good practice? Doesn't panics crash the whole program unless recover() is used?
If you don't want to use panic I have 3 other options you can use:

  1. Use panics with recover. But also you need to change the function to func (bot *BotAPI) UploadFiles(endpoint string, params Params, files []RequestFile) (apiRes *APIResponse, err error). In the recover, set err = errors.New(fmt.Sprintf("%v", r)); So the function will return with an error. Note that you can't return values from a "defered" function. Here is an code sample.
  2. Use an external error variable. For example before starting the goroutine, define a variable internalError (var internalError error). Then in the goroutine, if an error occurs, use internalError to store the error and then use return to stop the goroutine. Check the error after the line resp, err := bot.Client.Do(req). I just realized that this is a bad idea. If you do this the post request will be done (submitted to Telegram servers) without any errors and returning the error won't stop it.
  3. Use PipeWriter.CloseWithError; I'm not sure if this one is a good practice or not, but you can use w.CloseWithError(err) and then simply return from the goroutine. The error will be caught at resp, err := bot.Client.Do(req)

@Syfaro
Copy link
Member Author

Syfaro commented Jul 26, 2020

Whoops, I forgot to mention that the panics needed to be removed still.

I wasn't aware of CloseWithError, that seems like a perfect solution, thank you!

@Syfaro Syfaro mentioned this pull request Nov 6, 2020
32 tasks
@abserari
Copy link

abserari commented Jan 8, 2021

waiting for that

@Syfaro Syfaro marked this pull request as ready for review February 20, 2021 07:31
@Syfaro Syfaro mentioned this pull request Feb 22, 2021
@Syfaro Syfaro linked an issue Feb 22, 2021 that may be closed by this pull request
Copy link
Collaborator

@tjhorner tjhorner left a comment

Choose a reason for hiding this comment

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

This looks good to me, but what are we going to do about the breaking changes, since the versioning is really weird at the moment?

@@ -96,7 +95,7 @@ func buildParams(in Params) (out url.Values) {
}

// MakeRequest makes a request to a specific endpoint with our token.
func (bot *BotAPI) MakeRequest(endpoint string, params Params) (APIResponse, error) {
func (bot *BotAPI) MakeRequest(endpoint string, params Params) (*APIResponse, error) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This smells like a breaking change to me. People running off of master or develop are going to have a bad time.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah. Not really sure what else to do though, I want to keep consistency through everything and there's already a ton of breaking changes elsewhere so might as well do it now.

@Syfaro Syfaro linked an issue Mar 10, 2021 that may be closed by this pull request
@Syfaro Syfaro merged commit badaa40 into develop Mar 10, 2021
@Syfaro Syfaro deleted the multiple-uploads branch March 10, 2021 19:46
CNA-Bld added a commit to CNA-Bld/telegram-bot-api that referenced this pull request Feb 21, 2022
go-telegram-bot-api#356 updates the logic to support uploading multiple files but accidentally breaks this.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants