Skip to content
This repository has been archived by the owner on Jan 23, 2025. It is now read-only.

Image Forwarding #6

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

abalasky
Copy link

@abalasky abalasky commented Mar 4, 2022

For #1

"""
First pull request ever so not sure what convention is or what is generally required. Open to feedback. Will detail code changes here...

Also disregard branch name. This is for image forwarding.
"""

Wasn't able to test but I add ImageEvent to domain/domain.go that contains ImageBytes[].

These are obtained from whatsapp.ImageMessage.download() which is called in newly added HandleImageMessage() in events_provider.go

This is then passed to the telegram api and uploaded in similiar manner to QR code.

Hope this works!

@abalasky abalasky changed the title Abalasky tests for events handler Image Forwarding Mar 4, 2022
Copy link
Owner

@dstdfx dstdfx 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 the PR, good job working on this.
I noticed that there's no support for ImageMessageEvent in manager, let's add it too:
https://github.com/dstdfx/twbridge/blob/main/internal/manager/clients_manager.go#L55

go.mod Outdated Show resolved Hide resolved
internal/handler/events_handler.go Outdated Show resolved Hide resolved
internal/handler/events_handler.go Outdated Show resolved Hide resolved
internal/whatsapp/events_provider.go Outdated Show resolved Hide resolved
internal/whatsapp/events_provider.go Outdated Show resolved Hide resolved
internal/whatsapp/events_provider.go Show resolved Hide resolved
internal/whatsapp/events_provider.go Show resolved Hide resolved
.gitignore Show resolved Hide resolved
@abalasky
Copy link
Author

abalasky commented Mar 6, 2022

I addressed all the requested changes and pushed. However when I added the new case in clients_manager.go for ImageMessageEvent I now get a linting error regarding cyclomatic complexity:

internal/manager/clients_manager.go:42:1: cyclomatic complexity 31 of func (*Manager).Run is high (> 30) (gocyclo)
func (mgr *Manager) Run(ctx context.Context) {

@dstdfx
Copy link
Owner

dstdfx commented Mar 7, 2022

I addressed all the requested changes and pushed. However when I added the new case in clients_manager.go for ImageMessageEvent I now get a linting error regarding cyclomatic complexity:

internal/manager/clients_manager.go:42:1: cyclomatic complexity 31 of func (*Manager).Run is high (> 30) (gocyclo) func (mgr *Manager) Run(ctx context.Context) {

We can divide this Run method into smaller ones but I'd keep it in a separate PR for the same of simplicity.

For now, we can just fix the limit by adding this to the .golangcilint.yml:

linters-settings:
  gocyclo:
    min-complexity: 42

}

if err := eventsHandler.HandleImageMessageEvent(e); err != nil {
mgr.log.Error("failed to handle text message event", zap.Error(err))
Copy link
Owner

Choose a reason for hiding this comment

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

text -> image

return fmt.Errorf("failed to send image message chat_id=%d: %w",
event.ChatID,
err)
return fmt.Errorf("failed to notify telegram: %w", err)
Copy link
Owner

Choose a reason for hiding this comment

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

Let's keep failed to send image message, the idea is to extend the context of the error, this would save us time later if we need to debug it


// Download media data
mediaData, err := message.Download()
if err != nil {
wh.log.Error("got error", zap.Error(err))
Copy link
Owner

Choose a reason for hiding this comment

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

Let's be more specific here, for example: "failed to download media data" + error field

@abalasky
Copy link
Author

Sorry just returning to this but. I don't see a .golangcilint.yml . Only .golangci.yml. Should i create the dot file or add to .golanci ?

@dstdfx
Copy link
Owner

dstdfx commented Mar 22, 2022

.golangcilint.yml

You're right, it's called .golangci.yml, the piece of configuration that I sent should be located in this file:
https://github.com/dstdfx/twbridge/blob/main/.golangci.yml

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants