In Go, a statement can precede a conditional in an if-statement. Refactor structures like this to use those to constrain the scope of variables to the if-else-blocks where appropriate.

I.e.,

diff --git a/command.go b/command.go
index 3dc4f75..94da1a5 100644
--- a/command.go
+++ b/command.go
@@ -46,8 +46,7 @@ func CheckKarma(ctx *Context) {
                sep = " "
        }
 
-       _, err := s.ChannelMessageSend(m.ChannelID, reply.String())
-       if err != nil {
+       if _, err := s.ChannelMessageSend(m.ChannelID, reply.String()); err != nil {
                log.Printf("Error when sending reply to channel: %s\n", err)
        }
 }

None of these types are exported outside of Popple and therefore, to be consistent, all types should be made package-private.

To do this, just look for functions, types, and constants that start with an uppercase letter and make it a lowercase
letter.

This way, people can ask the bot what version it is.

I.e., @Popple version

Persion) @Popple version
Popple) 1.0.0

Basically, the opposite of the "top" command

Right now, uptime prints a time.Duration with a huge fractional component. That level of precision is unnecessary. Truncating to a time.Second should allow us to print something like 1h45m3s instead of 1h45m3.234234s.

$ go doc time.Duration.Truncate
package time // import "time"

func (d Duration) Truncate(m Duration) Duration
    Truncate returns the result of rounding d toward zero to a multiple of m. If
    m <= 0, Truncate returns d unchanged.

This way, the columns would line up nicely.

I.e.,

23423 hi
123 potato

into:

23423 hi
  123 potato

That is the equivalent of a for-loop with a val, ok := <- someChannel and then we could do without the break statement.

From this message:

error[E0277]: the trait bound `&T: codicon::Read` is not satisfied
    --> src/attest/qemu/mod.rs:62:50
     |
62   |         let query_sev: qmp::Return = from_reader(self).unwrap();
     |                                                  ^^^^ the trait `codicon::Read` is not implemented for `&T`
     | 
    ::: /home/ckuehl/.cargo/registry/src/github.com-1ecc6299db9ec823/serde_json-1.0.59/src/de.rs:2499:8
     |
2499 |     R: crate::io::Read,
     |        --------------- required by this bound in `from_reader`
     |
     = note: `codicon::Read` is implemented for `&mut T`, but not for `&T`
help: consider extending the `where` bound, but there might be an alternative better way to express this requirement
     |
59   |     T: std::io::Read + std::io::Write, &T: codicon::Read
     |                                      ^^^^^^^^^^^^^^^^^^^

Popple says:

------------- has -1 karma.

Likely need to switch from using Message.ContentWithMentionsReplaced to ContentWithMoreMentionsReplaced

Should be sufficient to just use the sql driver and sql statements. I would still like to retain the AutoMigrate functionality if possible.

Comment:

# docker image run --rm -v path/to/db:/root/popple.sqlite \
#                       -v path/to/token:/root/bot.token \
#                       image_name

Should be:

# docker run --rm -v path/to/db:/root/popple.sqlite \
#                       -v path/to/token:/root/bot.token \
#                       image_name

This will replace parts of the README.md. This should be a task-driven how-to document that describes:

• A pointer to how to create a Discord bot and get a bot token
• Deploying Popple as a container
• Deploying Popple as a systemd unit
• Backing up the Popple database
• and other helpful things if thought of later

2021/08/13 06:46:44 [DG0] wsapi.go:82:Open() error connecting to gateway wss://gateway.discord.gg/?v=8&encoding=json, dial tcp: lookup gateway.discord.gg on 67.207.67.3:53: read udp 10.88.0.8:57565->67.207.67.3:53: read: no route to host
2021/08/13 06:46:44 [DG0] wsapi.go:835:reconnect() error reconnecting to gateway, dial tcp: lookup gateway.discord.gg on 67.207.67.3:53: read udp 10.88.0.8:57565->67.207.67.3:53: read: no route to host

More investigation is needed. Could be Popple, discordgo, container network configuration, or host network configuration. Probably not Discord's API, since my friend's bot in that same server seemed to have remained online.

The worker shutdown is kind of onerous. Instead of tracking how many workers have returned, use a sync.WaitGroup instead.

However, it's not sufficient to just wait for wg.Done() since that will prevent us from quitting even if the deadline has passed, so you might consider something like:

    workersDone := make(chan struct{})
    go func() {
        wg.Wait()
        close(workersDone)
    }()
    
    select {
        case <- deadline:
            // emit warning blah
        case <- workersDone:
            // emit that all workers are done
    }

Any application-specific implementation details should be handled by the respective cmd/implementation/popple command. The top-level Popple package should provide a good-enough interface such that it doesn't need to know about anything implementation-specific.

Rename the command.Request struct's GuildID field to ServerID.

Parsing will have to be fixed.

meme-bot++ or @meme-bot++ should work.

Discord seems to be rolling out support for threads and it appears Popple doesn't react to messages in threads that it is a part of.

Things to keep an eye on upstream:

• bwmarrin/discordgo#922
• bwmarrin/discordgo#982

This is probably not a portable Makefile, I'm sure there's a more idiomatic way to glob all of the *.go files.

• Run golint and fix any warnings
• Tackle any issues with the refactoring or good first issue labels
• Add test cases to files ending in _test.go
• Add doc comments to exported symbols in packages
• Run your own instance of the Popple bot and report any issues or feature requests

Popple currently depends on my fork of discordgo where I implemented most of the support for threads (but as far as Popple is concerned, 100% of its thread needs are met.) I am not upstreaming my patch since another developer has a PR in-progress which includes thread support among many other features.

This release blocker is commit 580df4f.

Choose some default value for the number of buffered items, say 16. There's no reason to use an unbuffered channel since these goroutines don't necessarily HAVE to synchronize. Let them get work done instead of blocking.

See the FIXMEs in func SetAnnounce.

These packages should be replaced with non-cgo versions:

• go-sqlite3

Right now, the "deadline" flag is a uint and converted to a Duration later on.

    timeout := flag.Uint("deadline", 3, "seconds to wait for workers to exit when shutting down")

It looks like the flag package supports Duration types out of the box (rest of output omitted for brevity)

$ go doc flag
func Duration(name string, value time.Duration, usage string) *time.Duration

Change it.

Go 1.17 comes with some performance improvements due to passing arguments
via registers instead of just the stack.

More info on this: https://golang.org/doc/go1.17#compiler

Development has slowed down and Popple has stabilized, but has changed a lot since ARCHITECTURE.md was written.

Update it to show how various components of Popple work. Things like how it is a primarily event driven application, the work queue, the router, the purpose of the _testing.go files.

Use some default value, say 32, when allocating the slice in ParseSubjects. This will reduce the number of memory allocations needed for most messages.

popple.go:25:7: don't use ALL_CAPS in Go names; use CamelCase
popple.go:32:7: don't use ALL_CAPS in Go names; use CamelCase

With build, test -v and go fmt jobs.

Being able to chain arbitrary amounts of increments and decrements is kind of an anti-feature and useless.

I.e., remove the ability to do Popple++--++ = Popple += 2. Just parse a number of ++ or a number of -- but not both.

Go 1.17 comes with some performance improvements due to passing
arguments via registers instead of just the stack.

Upgrade the Dockerfile's build stage to base on golang:1.17-alpine
instead of golang:1.15-alpine so that new builds can take advantage of
this.

More info on this: https://golang.org/doc/go1.17#compiler

Right now, the router's route method just uses strings.HasPrefix to determine if the message contains a command.

This means it's possible to concatenate a valid command with garbage:

Person) @Popple helpasdf
Popple) *sends help text*

It's not exactly a bug, since it won't technically hurt anything, but "helpasdf" is also technically not "help".

Fix this.

This file could probably be called model.go or data.go or something.

Person) @Popple uptime
Popple) 2 weeks, 4 days, and 12 hours

The format doesn't have to be the same in the example above.

• Add func uptime(req request, rsp responseWriter, db *gorm.DB, started time.Time) to command.go
• Add a route to the router in popple.go can probably get time.Now() in func main and access it via the closure passed into addRoute
• Add unit tests for func uptime in command_test.go
• Document this in README.md

I.e., these two should be equal:

You) @Popple version
Popple) Blah blah

You) version
Popple) Blah blah

The Channel.Type field may be useful.

It should not. Poe++ Hello is valid though.

func ParseSubjects won't include subjects with zero karma in its result but func CheckKarma relies on this behavior.

Right now, the workaround is to invoke the karma subcommand and put an arbitrary increment/decrement against each subject:

Person) @Popple karma asdf++ potato--
Popple) asdf has 3 karma. potato has 0 karma.

This will require changes to most of parse.go.