Fix client.ExportStringSession() and add RunMiddleware field to client.
#41
Conversation
| @@ -123,6 +123,20 @@ type ClientOpts struct { | |||
| ErrorHandler dispatcher.ErrorHandler | |||
| // Custom middlewares | |||
| Middlewares []telegram.Middleware | |||
| // Custom Run() Middleware | |||
| RunMiddleware func( | |||
There was a problem hiding this comment.
I can't think of any potential need of this middleware function, can you tell us what made you add this?
There was a problem hiding this comment.
Overlay Run function(needed for FloodWait middleware)
https://pkg.go.dev/github.com/gotd/contrib/middleware/floodwait
There was a problem hiding this comment.
Makes sense, I'd suggest to rename (L 128) origRun to runFunc with respect to naming scheme throughout the library.
client.go
Outdated
| @@ -135,6 +149,9 @@ func NewClient(appId int, apiHash string, cType ClientType, opts *ClientOpts) (* | |||
| } | |||
|
|
|||
| ctx, cancel := context.WithCancel(context.Background()) | |||
| if opts.Ctx != nil && opts.CtxCancel != nil { | |||
| ctx, cancel = opts.Ctx, opts.CtxCancel | |||
There was a problem hiding this comment.
How does this external context and cancel function helps and what problem does it solve?
Also, you should consider adding the original ctx and cancel declaration of the library in an else condition as the created cancellable context would never be used in case the condition present in line 152 comes out to be true.
There was a problem hiding this comment.
According to the idea, this is needed to create a time out, before, if the account was broken, the client would wait forever, has this been fixed now? I did this to transmit a 60-second timer there. Maybe you have an idea on how to do this better? It’s just that my decision is dirty, I admit, I can’t find out the reason for the failure, and this is important! Please help with the solution, you, as a library designer, probably know whether the client will hang forever or time out, and is there such a thing at all or is there some other processing mechanism?
There was a problem hiding this comment.
I think it would be better to just take Context as optional input argument rather than taking Context and a CancelFunc as cancel function can be derived later by the library itself to ease it for the user.
Something like:
if opts.Context == nil {
opts.Context = context.Background()
}
ctx, cancel := context.WithCancel(opts.Context)What do you think about it?
client.go
Outdated
| c.initialize(&wg), | ||
| ) | ||
| } | ||
|
|
||
| c.err = c.Run(c.ctx, c.initialize(&wg)) |
There was a problem hiding this comment.
You should consider removing this because you are already calling the c.Run function above it.
Please clarify the need of RunMiddleware before proceeding to this change.
There was a problem hiding this comment.
@celestix The thing is that I need access to the flood wait middleware, now I have removed it in my code, but I need to make my own handler and most likely I will still need access to this method. If you think this is freaky, then suggest how I can do this. I launch 10 clients, put them in an array, launch consumers for AMQP and put tasks there, for example, join a chat, and at the moment the task is executed, the client catches a flood wait, each client is launched according to the session that is in the Account table, I need to tag this account like floodwaited for time (and there is a time when floodwait will finish its work), so if you can offer an idea on how I can do this without wrapping RunMiddleware, then I will be glad. I myself think this is a crutch and dirty code, we both want to see this project clean) But it is closed for extension, and we need to think together how to solve this. we can discuss this in chat
celestix
changed the title
client.ExportStringSession() and add RunMiddleware field to client.
client.go
Outdated
| @@ -135,6 +149,9 @@ func NewClient(appId int, apiHash string, cType ClientType, opts *ClientOpts) (* | |||
| } | |||
|
|
|||
| ctx, cancel := context.WithCancel(context.Background()) | |||
| if opts.Ctx != nil && opts.CtxCancel != nil { | |||
| ctx, cancel = opts.Ctx, opts.CtxCancel | |||
There was a problem hiding this comment.
I think it would be better to just take Context as optional input argument rather than taking Context and a CancelFunc as cancel function can be derived later by the library itself to ease it for the user.
Something like:
if opts.Context == nil {
opts.Context = context.Background()
}
ctx, cancel := context.WithCancel(opts.Context)What do you think about it?
| @@ -123,6 +123,20 @@ type ClientOpts struct { | |||
| ErrorHandler dispatcher.ErrorHandler | |||
| // Custom middlewares | |||
| Middlewares []telegram.Middleware | |||
| // Custom Run() Middleware | |||
| RunMiddleware func( | |||
There was a problem hiding this comment.
Makes sense, I'd suggest to rename (L 128) origRun to runFunc with respect to naming scheme throughout the library.
No description provided.