narwhalirc / narwhal-plugin-solus

This is the Narwhal IRC plugin for the Solus community
Apache License 2.0
1 stars 0 forks source link

Add Phabricator Task (Tnnnn) and Diff (Dnnnn) parsing #1

Open ermo opened 5 years ago

ermo commented 5 years ago

Make it convenient to link to tasks and diffs with the normal narwhal bot dot syntax and leverage the existing UrlParser to display full link and title.

Example:

User input:

.T7892

or

.D6610

simple text replace (intermediate step):

IF the task or diff exists, NarwhalBot outputs:

@NarwhalBot | https://dev.getsol.us/T7892

or

@NarwhalBot | https://dev.getsol.us/D6610

which is also UrlParsed normally to:

@NarwhalBot | [ ⚓ T7892 Update Samba to the 4.9.x branch (from the 4.8.x branch) ]

or

@NarwhalBot | [ ⚙ D6610 samba: Update to 4.9.9 ]

ELSE NarwhalBot does nothing.

This regexp might work:

task_or_diff = regexp.MustCompile("^(D|T)([0-9]+)$")

The dot prefix ensures that NarwhalBot is only triggered on purpose, which I would personally consider a feature, not a bug. Furthermore, the requirement that the entire command must consist only of a task or diff makes it easy to use simple text replacement.

EbonJaeger commented 5 years ago

When Narwhal sends a message to the channel, does it then parse its own message, or is that something that has to be done by hand in this module?

Instead of doing nothing when no task/diff is found, it would probably be better for it to actually say that it didn't find anything. This immediately lets the user know that something is wrong with their input. Currently, Narwhal does nothing on bad or non-existent URLs, either.

JoshStrobl commented 5 years ago

When Narwhal sends a message to the channel, does it then parse its own message, or is that something that has to be done by hand in this module?

No. We explicitly ignore our own messages as bot.

Instead of doing nothing when no task/diff is found, it would probably be better for it to actually say that it didn't find anything.

I feel like that could get spammy pretty quickly, especially if we opt for substring checking of task and diff IDs instead of it being on a dedicated line.

Furthermore, the requirement that the entire command must consist only of a task or diff makes it easy to use simple text replacement.

I'd actually prefer it parse both whole lines as well as substring matches to allow multiple links to be parsed at once.

which is also UrlParsed normally to:

Actually the way we'd do it is a bit inverse. We'd check for matches for the diff or task IDs, expand that to a full URL, then have sauron parse the URL and we just handle the result.

ermo commented 5 years ago

I'd actually prefer it parse both whole lines as well as substring matches to allow multiple links to be parsed at once.

Do you want to restrict parsing to e.g. .(phab|dev) T7892 D6610 (so still a deliberate command prefix to ask narwhal-bot to parse for tasks/diffs) or do you want to parse every line everyone writes willy nilly for task/diff substitutions?

FWIW, I have experience with the latter -- it can get a bit annoying at times, but it can also be very convenient if used responsibly.

Actually the way we'd do it is a bit inverse. We'd check for matches for the diff or task IDs, expand that to a full URL, then have sauron parse the URL and we just handle the result.

I was hoping for something like this. =)

JoshStrobl commented 5 years ago

My plan is to parse every line for the differential and task IDs, then expand those out to the full URL, check if it's a valid page, and if so return the details on it and the URL itself. It'll be developed in a way where we should, in theory, be able to support passing multiple task or diff IDs, mixing the two as needed, etc. and we'll check each one individually. We'll ideally want to do checks for whether or not it is at the beginning of the line, whether it has a prefixed whitespace, and ensure it isn't part of a URL already.

If we find certain people to be abusing it, I can always have that line checking also match the NarwhalMessage.Issuer to see if they're blacklisted, but I doubt we'll get to that point. Alternatively we can just talk to them about it.

Anyways, I laid the groundwork today (incorporating some regex for getting last synced ISO 8601 timestamp and refactoring the commands so we can handle instances where no command is provided, such as ya know..normal human language). So pretty confident I can get this integrated in next couple days.