Plugin system restyling #15

NeroReflex · 2016-09-13T13:48:21Z

Cambiamenti STRUTTURALI:

i plugin tornano a NON avere tipo, quindi un plugin unisce entrambe le tipologie.
il parsing del messaggio va fatto a mano (anche se il vecchio parsing rimane disponibile per retro-compatibilità e perché può far comodo ai neofiti)
ogni plugin lancia ben DUE Threads (per evitare che i due meccanismi di plugin si limitino fra loro)
le funzioni si lanciano così:
!convert hex2bin 1aff5
una facile ed intuitiva guida si invoca così: !help
la guida di uno specifico plugin si invoca così: !help convert
maggiore velocità a runtime, soprattutto nel main thread (parte critica in Pizza)

Ora un plugin NON ha più un tipo e ha un'unica coda di richieste. Ogni richiesta ha un tipo (in sostituzione del tipo di plugin). Ogni plugin lancia (al massimo) DUE thread (uno per tipo di richiesta). Cambiata l'interfaccia per rispondere alle richieste.

doppioandante · 2016-09-13T14:32:12Z

src/main/java/com/neroreflex/pizza/Pizza.java

+            } else if (command.compareTo("help") == 0) {// E se non lo e' invia l'errore
+                this.Help(new Request(channel, sender, rawParamsString));
+            } else {
+                this.enqueueMessage(new Message(channel, "I don't know what I have to do at '" + command + "' request :("));


Nel caso il comando non esista secondo me è più chiaro non scrivere nulla.

!time
dopo un ipotetico secondo.....
ma è morto o il plugin non esiste?

!help dà la risposta

Non lo so.... Mi sembra brutto (e un po' maleducato)....

doppioandante · 2016-09-13T14:47:12Z

Ci ho dato uno sguardo molto veloce, una cosa che mi preme è che con @gianluca-nitti avevamo fatto in modo che il bot di hackernews andasse a intervalli regolari ma a partire da ore prestabilite, cosa che si è persa quando hai messo l'autotrancio.
Non ho ancora guardato cosa intendi per 'ogni plugin lancia ben DUE Threads' ma a sentirla così non sembra una grande cosa, guarderò meglio se avrò tempo.

Risposte

La possibilità di far partire l'onPoll a intervalli regalari a partire da ore prestabilite è stata introdotta per tutti i plugin.

DUE Thread: uno per il timer e uno per la blockingQueue che gestisce l'onCall. Avere un solo thread significherebbe accodare onCall e onPoll o spendere più tempo a runtime per capire quale eseguire,
ma come è ora è più leggibile.

I thread vengono eseguiti PRIMA di settare loaded, quindi isLoaded() ritorna un valore dipendente dal tempo di scarto tra inizio esecuzione thread e fine caricamento plugin. In pratica un plugin funzionerà se e solo se ha fortuna!

Implementato un modo per definere la prima chiamata ad onPoll. Il codice precedentemente usato per il plugin HackerNews è tornato, e funge anche da esempio per gli altri!

gianluca-nitti · 2016-09-19T21:09:41Z

@NeroReflex @doppioandante c'è qualche motivo particolare per cui non è ancora stato fatto il merge in master?

doppioandante · 2016-09-19T22:43:52Z

Perché @NeroReflex aveva trovato un problema che adesso non ricordo.
A me sinceramente non piace il discorso dei due thread, dovrebbero essercene due solo se il plugin richiede l'attivazione periodica.
E comunque ancora non mi convince del tutto che ci sia questo kitchen sink dentro il plugin, preferivo una soluzione come quella dell'autotrancio che aggiunge la funzionalità del polling estendendo la classe Trancio: prima l'architettura del tutto rendeva le due cose esclusive, ora invece si può benissimo implementare così.

Un'altra cosa che non ho capito a che scopo sia stata messa è DelayedRequest.

A parte questo non ho molto altro in contrario. In futuro mi piacerebbe che Request e Message venissero unificati perché non ha senso avere una distinzione semantica riguardo alla provenienza, è già implicita nella gerarchia di comunicazione bot/plugin.

doppioandante · 2016-09-21T20:23:04Z

src/main/java/com/neroreflex/pizza/Pizza.java

-        Matcher invokeMatcher = invokeRegex.matcher(message);
-        if (invokeMatcher.matches()) {
+        // Cerco di capire se mi e' stato dato un comando
+        if ((message.charAt(0) == '!') && (message.length() > 1)) {


Può essere che arrivi un messaggio di lunghezza nulla?

https://tools.ietf.org/html/rfc2812.html Sezione 2.3.1: "Empty messages are silently ignored"

doppioandante · 2016-09-21T20:24:04Z

src/main/java/com/neroreflex/pizza/Pizza.java

            // Ottieni il nome del comando
-            String command = invokeMatcher.group(3).toLowerCase();
+            int firstSpacePos = message.indexOf(' ');
+            firstSpacePos = (firstSpacePos <= -1)? message.length() : firstSpacePos;


Nit: di solito ci va lo spazio prima del ? del ternario, ma lascia pure così.

doppioandante · 2016-09-21T20:26:45Z

src/main/java/com/neroreflex/pizza/Pizza.java

+            } else if (command.compareTo("info") == 0) {// E se non lo e' invia l'errore
+                this.Info(new Request(channel, sender, rawParamsString));
+            } else {
+                this.enqueueMessage(new Message(channel, "I don't know what I have to do at '" + command + "' request :("));


La grammatica corretta sarebbe "to do with", e probabilmente quel "request" alla fine va tolto.

doppioandante · 2016-09-21T20:27:51Z

src/main/java/com/neroreflex/pizza/Pizza.java

+            this.enqueueMessage(new Message(helpRequest.GetChannel(), "My source code <3 https://github.com/NeroReflex/Pizza"));
+        } else {
+            this.enqueueMessage(new Message(helpRequest.GetChannel(), "No info about that"));
+        }


Penso che sarebbe più semplice far stampare semplicemente I'm blabla + url github (com'era prima forse?), è una complicazione inutile avere dei subcommands per info.

doppioandante · 2016-09-21T20:30:12Z

src/main/java/com/neroreflex/pizza/Pizza.java

+
+                        // Stampo la guida solo se ha senso (la stringa della guida NON E' vuota)
+                        if (!help.isEmpty())
+                            this.enqueueMessage(new Message(helpRequest.GetChannel(), "!" + name + " " + help));


Per ora OK, ma il si dovrà cambiare in modo che il messaggio con le informazioni di help arrivi in privato all'utente che l'ha richiesto, altrimenti intasa la chat (specie con molti plugin o con help lunghi).

doppioandante · 2016-09-21T20:30:36Z

src/main/java/com/neroreflex/pizza/Pizza.java

+                if (!help.isEmpty())
+                    this.enqueueMessage(new Message(helpRequest.GetChannel(), "!" + name + " " + help));
+                else
+                    this.enqueueMessage(new Message(helpRequest.GetChannel(), "Help is not bundled with the '" + name + "' plugin."));


(There's) No help is provided by

doppioandante · 2016-09-21T20:50:07Z

src/main/java/com/neroreflex/pizza/Trancio.java

+            throw new UnsupportedOperationException("Not supported yet."); //To change body of generated methods, choose Tools | Templates.
+        }
+
+    };


RequestRunner e TimerRunner si potevano fare come classi anonime ma va bene anche così.

doppioandante · 2016-09-21T20:52:38Z

src/main/java/com/neroreflex/plugins/Join.java

        } else {
-            this.sendMessage(new Message(channel, user + "Please specify a channel"));
+            this.sendMessage(new Message(channel, user + " Invalid channel name"));


Questo else avviene quando non si dà nessun canale, quindi ha più senso il messaggio che c'era prima.

doppioandante · 2016-09-21T20:53:07Z

src/main/java/com/neroreflex/plugins/Join.java

            // Chiave del canale?
            String key = ((args.size() > 1) && (args.get(1).length() > 0))? args.get(0) : "";

            this.joinChannel(newChannel, key);
-            this.sendMessage(new Message(channel, user + " I have joined " + newChannel));
+            this.sendMessage(new Message(channel, user + " I have joined the " + newChannel + " channel"));


Il messaggio di prima era meno verboso e si capisce ugualmente per via del # prima del nome del canale.

NeroReflex added 2 commits September 13, 2016 02:39

Migliore usabilità

53ffd1c

doppioandante reviewed Sep 13, 2016
View reviewed changes

NeroReflex added 3 commits September 13, 2016 20:30

I thread vengono eseguiti PRIMA di settare loaded

2a2195b

I thread vengono eseguiti PRIMA di settare loaded, quindi isLoaded() ritorna un valore dipendente dal tempo di scarto tra inizio esecuzione thread e fine caricamento plugin. In pratica un plugin funzionerà se e solo se ha fortuna!

I commenti come questo sono un po' lapalissiani.

65f9658

Recuperato codice da HackerNews e reso API

3dc8304

Implementato un modo per definere la prima chiamata ad onPoll. Il codice precedentemente usato per il plugin HackerNews è tornato, e funge anche da esempio per gli altri!

NeroReflex added 2 commits September 21, 2016 22:05

Rimosso il messaggio di cattivo gusto

7a059bc

Non più necessario

60b5e56

doppioandante requested changes Sep 21, 2016

View reviewed changes

doppioandante approved these changes Sep 21, 2016

View reviewed changes

L'help risponde in privato.

4d021e8

doppioandante approved these changes Sep 21, 2016

View reviewed changes

HackerNews torna (meglio) di come era prima

854a13d

NeroReflex merged commit 999235a into master Sep 21, 2016

NeroReflex deleted the PluginSystem-restyling branch September 21, 2016 22:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Plugin system restyling #15

Plugin system restyling #15

NeroReflex commented Sep 13, 2016

doppioandante Sep 13, 2016

NeroReflex Sep 13, 2016

doppioandante Sep 13, 2016

NeroReflex Sep 13, 2016

doppioandante commented Sep 13, 2016 •

edited by NeroReflex

gianluca-nitti commented Sep 19, 2016

doppioandante commented Sep 19, 2016

doppioandante Sep 21, 2016

NeroReflex Sep 21, 2016

doppioandante Sep 21, 2016

doppioandante Sep 21, 2016

doppioandante Sep 21, 2016

doppioandante Sep 21, 2016

doppioandante Sep 21, 2016 •

edited

doppioandante Sep 21, 2016

doppioandante Sep 21, 2016

doppioandante Sep 21, 2016

Plugin system restyling #15

Plugin system restyling #15

Conversation

NeroReflex commented Sep 13, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

doppioandante commented Sep 13, 2016 • edited by NeroReflex

Risposte

gianluca-nitti commented Sep 19, 2016

doppioandante commented Sep 19, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

doppioandante Sep 21, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

doppioandante commented Sep 13, 2016 •

edited by NeroReflex

doppioandante Sep 21, 2016 •

edited