Fix some errors in the batch module and its usage

[guyguy2001]

The problems I fixed:

• Inconsistent usage of *batchParameters and batchParameters made it so when we send a string as a parameter (which is what you've done on 0.5), it splits it as if it was an array of parameters; In my case it resulted in # g e n e r a l
• The BATCH command should not include the username it is sent to, nor the batch id (specs)

Two things I would like feedback on:

• My usage of IRCBase seems a bit suspicious; I do mean to send it just to that user, so it may be the right thing to do, but I would still like a preview to make sure there's not a better way to do that.
• I added the * to batchParameters based on what made sense to me (and based on how you called it from the history module in 0.5), but I'm not a python programmer - so I may have been wrong on how I used that.

[ElementalAlchemist]

Thank you!
This is something I'd already fixed in 0.5 if you want to reference those commits (2d23885, 27f2498, ba7333e).

As for feedback on your change:
The batch parameters are intended to be a list (rather than a parameter list). I'm not sure how much I care about that in 0.4, but in 0.5 there were already some places correctly referencing it as a list by the time I'd gotten to fixing the chathistory batch (which I didn't write, and apparently I missed in review). I might ask this stay that way anyway since 0.5 should be merging fairly soon, and I'd like the merge process to be as painless as possible. In general, I don't know that I actually care that much for interfaces that rely on passing an arbitrary parameter list; IRCBase was designed as a mostly drop-in replacement for twisted.words.protocols.irc.IRC, but I think if/when we do another version of txircd that I should change up the interface of sendMessage to one that makes a bit more sense and is harder to screw up in cases like this (right now it's really only ideal for the 90% case, which is good, but its design makes it too easy for the other 10% to slip through).

That usage of IRCBase is a little bit sketchy. In particular, it's worth noting that there are other user classes that handle sendMessage differently than converting it into a protocol message and dropping it on the wire, which that usage of IRCBase would override. RemoteUser sort of prevents that from happening since there's not really a wire to drop it onto (it once forwarded messages onto the appropriate server, but I'd decided at some point it was better to be explicit about sending server messages). LocalUser has some fun handling of its own, though it's not used yet by any released modules in 0.4. As in the 0.5 commit, txircd has a to parameter of the message, which is what you want to manipulate here. As I mentioned above, this is probably something I'd try to change with a new sendMessage interface, but for now, this is what we have.

[guyguy2001]

Thanks for the feedback, and sorry for the late response;
It seems like 0.5 still has the bug that makes the BATCH command send the batch tag, which it shouldn't:
https://github.com/ElementalAlchemist/txircd/blob/2d23885da9701688c9162747d0199016782296d2/txircd/modules/ircv3/batch.py#L42-43
vs
https://github.com/ElementalAlchemist/txircd/pull/162/files#diff-a494b100942efd27341c542521e7cecaR43-L43
I wasn't able to run txircd 0.5 yet due to a couple of issues in my WSL, so I couldn't test it yet, but from comparing it to my python 2 tests it seems like that would still be an issue.
If you want I can run test it on 0.5 later this week.

Also, one thing I don't understand the asymmetry between startBatch and sendMessage; Shouldn't either both of them or neither of them have a *?

And either way - would you like me to close this issue, or only keep the changes that remove the username from the BATCH command?

[ElementalAlchemist]

Apologies for my late response as well.

I'm not sure what you're looking at to say that BATCH is sending the BATCH tag; the code you linked doesn't send any tags with the BATCH message. It does include it as a BATCH parameter, which is required in the spec so the client knows which batch is being opened/closed. We also store it in the user cache so we can tag it on the messages we're sending in the batch.

Regarding the asymmetry between startBatch and sendMessage: yes, it's odd that they're different. I don't think we're at the point in the release where I'd align them. If I was to fix the issue, I'd rather change sendMessage, and doing that isn't going to happen at the beta point of the software (it's used everywhere and too easy to screw up). There are also other current callers to startBatch that already call it correctly (i.e. with its current parameter list).

If it's still useful in the time you'll be running 0.4, you could keep the changes that remove the username from BATCH if you wish, or I could cherry-pick the 0.5 commit back for you.

[luxaritas]

I'm not sure what you're looking at to say that BATCH is sending the BATCH tag; the code you linked doesn't send any tags with the BATCH message. It does include it as a BATCH parameter, which is required in the spec so the client knows which batch is being opened/closed.

I looks like the issue is that user.cache["currentBatch"] = uniqueReferenceTag is set before user.sendMessage("BATCH", ...), which is causing the BATCH tag to be erroneously included (since the modifyoutgoingmessage callback will see it in the cache)

[ElementalAlchemist]

Oh, whoops. I see, you're right. I pushed a fix for that to the 0.5 branch.