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

Tag mantainer #132

Merged
merged 22 commits into from
Nov 25, 2023
Merged

Tag mantainer #132

merged 22 commits into from
Nov 25, 2023

Conversation

Jabbar03
Copy link
Contributor

No description provided.

@ali-benny
Copy link
Member

In realtà, come scritto in #93 sarebbe più pratico che si inoltrasse automaticamente il messaggio in cui ci hanno taggato sul gruppo csunibo

@samuelemusiani
Copy link
Member

samuelemusiani commented Nov 17, 2023

In realtà, come scritto in #93 sarebbe più pratico che si inoltrasse automaticamente il messaggio in cui ci hanno taggato sul gruppo csunibo

Giustamente sarebbe più efficace in quanto non è detto che i maintainers siano in tutti i gruppi con il bot. Il fatto è che @csunibo indica già il nome del gruppo quindi il bot interverrebbe tutte le volte che si indica semplicemente il gruppo. Per risolvere si potrebbe inverare un altro tag o propriamente un comando (tipo /issue) che inoltra direttamente la il messaggio citato

@Jabbar03 Jabbar03 self-assigned this Nov 17, 2023
@samuelemusiani
Copy link
Member

Inoltre inoltrare un messaggio sul gruppo di csunibo riporta al problema di hardcodare gli I'd dei gruppi all'interno del bot.

In realtà non credo sia un problema perché csunibo dovrebbe essere un gruppo statico che non cambia di anno in anno come quelli relativi ai vari anni

@foxyseta
Copy link
Member

Anche io sono a favore di usare la sintassi @ anziché /. La prima non è idiomatica, ha l'effetto collaterale di aprire la chat anziché rinviare il comando quando toccata da GUI e cosa più grave richiede che il bot abbia accesso in visualizzazione a tutti i messaggi della chat (non solo quelli che cominciano con /), cosa che è un privilegio non richiesto.

Ma se vogliamo inoltrare il messaggio a quel punto non è molto più pratico usare /csunibo e poi si apre il gruppo e ci scrive direttamente l'utente? Così possiamo sostenere una conversazione vera e propria.

@foxyseta
Copy link
Member

Forse il vantaggio di taggare direttamente dentro il gruppo originario è quello che si può osservare l'incidente a colpo d'occhio, ma è vero che non diamo garanzie di essere già dentro ai gruppi.

@samuelemusiani
Copy link
Member

samuelemusiani commented Nov 17, 2023

Non ho controllato le API ma forse c'è un metodo per vedere se un utente è dentro un gruppo. Si potrebbe fare che di predefinito si tagga e se nessuno dei maintainers è in quel gruppo si inoltra il messaggio

@foxyseta
Copy link
Member

Sicuramente un modo c'è sì. Però anziché inoltrare il messaggio penso sia meglio taggare @csunibo come piano B chiedendo di scrivere lì. Così abbiamo modo di fare domande di contesto subito

@foxyseta foxyseta changed the title Tag manteiner Tag mantainer Nov 17, 2023
@foxyseta foxyseta linked an issue Nov 17, 2023 that may be closed by this pull request
@samuelemusiani
Copy link
Member

samuelemusiani commented Nov 18, 2023

Ok direi allora che introduciamo un comando /issue che tagga i maintainers, se nessuno di quelli che verrebbe taggato è presente nel gruppo scriviamo di inoltrare il problema a @csunibo.

Che ne dite?

@foxyseta
Copy link
Member

Perfetto.Scusa @Jabbar03! Se vuoi la prossima volta puoi scrivere sulla issue su cui vorresti lavorare per chiedere più informazioni.

@samuelemusiani
Copy link
Member

@Jabbar03 al lavoro adesso per modificare la PR

@foxyseta
Copy link
Member

La cosa più importante è fare 0 hardcoding per me (NESSUN handle Telegram dentro i file .go). Se riesci io mettrei tutto in actions.json. Probabilmente ti servirà l'aiuto di qualche senpai tipo @bogo8liuk, @musianisamuele o me.

@Jabbar03
Copy link
Contributor Author

Ok mi metto al lavoro. Se ho domande chiedo ai senpai citati.

@Jabbar03 Jabbar03 closed this Nov 24, 2023
@Jabbar03 Jabbar03 deleted the tag_manteiner branch November 24, 2023 16:36
@Jabbar03 Jabbar03 restored the tag_manteiner branch November 24, 2023 16:46
@ali-benny ali-benny reopened this Nov 24, 2023
@foxyseta
Copy link
Member

Il primo controllo come hai notato falisce solo perché vuole che formatti il codice prima che la PR venga accettata. Se il secondo continua a fallire non preoccuparti ci guardo io alla fine.

@foxyseta foxyseta added the enhancement New feature or request label Nov 24, 2023
Copy link
Member

@samuelemusiani samuelemusiani left a comment

Choose a reason for hiding this comment

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

Ci sono un po' di cosa da cambiare

model/controller.go Outdated Show resolved Hide resolved
model/controller.go Outdated Show resolved Hide resolved
json/actions.json Outdated Show resolved Hide resolved
model/controller.go Outdated Show resolved Hide resolved
model/controller.go Outdated Show resolved Hide resolved
model/controller.go Outdated Show resolved Hide resolved
model/globals.go Outdated Show resolved Hide resolved
model/parse.go Outdated Show resolved Hide resolved
model/parse.go Outdated Show resolved Hide resolved
@samuelemusiani
Copy link
Member

samuelemusiani commented Nov 25, 2023

Adesso faccio la review, però non capisco perché i test falliscano. @foxyseta te hai qualche idea? Quello sul formatting di go a me in locale non da alcun tipo di problema.

EDIT: Direi che il problema del json è che manca ^M alla fine. Non ci avevo mai fatto caso che json utilizzasse gli end-of-line alla DOS/Windows

Copy link
Member

@samuelemusiani samuelemusiani left a comment

Choose a reason for hiding this comment

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

Tralasciando i test che non passano c'è solo la questione di come scrivere il campo data

model/globals.go Outdated Show resolved Hide resolved
json/actions.json Outdated Show resolved Hide resolved
@samuelemusiani
Copy link
Member

samuelemusiani commented Nov 25, 2023

Comunque per chiunque faccia il merge finale, per favore facciamo uno squash che siamo già a 20 commit per 80 righe di codice. Sono 1 commit ogni 4 righe ahaha

@Jabbar03 Jabbar03 merged commit bd33388 into main Nov 25, 2023
3 checks passed
@foxyseta foxyseta deleted the tag_manteiner branch November 25, 2023 23:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

@csunibo per taggare i mantainer
4 participants