boxes: Improve trigger sequence for generic_autocomplete

[preetmishra]

Updated generic_autocomplete along with its autocomplete_mentions/streams to provide typeahead even when there are some characters before their respective trigger character.

Fixes #541 and #448 (partially).

[kaustubh-nair]

LGTM!
Adding some test cases would be a good idea to ensure that something isn't amiss. :)

[preetmishra]

@kaustubh-nair Thanks for the review. I have pushed the amended tests now.

[preetmishra]

Update: I have added a refactor commit to make autocomplete_streams' signature consistent with autocomplete_mentions as per the suggestion of @kaustubh-nair. (#542 (review))

[sumanthvrao]

@preetmishra This looks fairly straightforward and much more consistent. The changes LGTM!

minor: Looks like the only place we are hard-coding the prefix string now is these group_typeaheads. Maybe we can do something along the same lines there as well.

[preetmishra]

@sumanthvrao Thanks. I have pushed a new commit to address the latter point.

[preetmishra]

The Travis CI build is failing with 'An error occurred while generating the build script.' for Install & pytest - Python 3 (OS X; xcode10). I tried to restart it with force push but it didn't work again.

[sumanthvrao]

@preetmishra thanks for addressing the minor feedback quickly. The additional commit LGTM!

@neiljp FYI :)

[preetmishra]

Update: Rebased to include autocomplete_emojis, that was added in 2c30caf, in the PR as well.

[neiljp]

@preetmishra Sorry for not getting back to this; could you rebase?

[preetmishra]

@neiljp No problem; I have rebased the PR. :)

The first commit was a suggestion from @sumanthvrao to improve consistency. The next two are self-explanatory refactor commits that facilitate the new change.

[kaustubh-nair]

@preetmishra I just noticed that you recently pushed a07c87d for refactoring generic_autocomplete. While it looks good but there is still a little bit of duplication. I had pushed similar changes to the footer PR.

I'd recommend holding off refactors to autocomplete methods for a while, since there are already so many of them in the footer PR. Adding them in different PRs will cause a lot of confusion.
That being said, if you want to make any changes, feel free to push to the autocomplete_footer branch on my fork. It would be better if we keep autocomplete refactors in the same place :)

[preetmishra]

@kaustubh-nair Thanks for taking a look. 👍

I see what you're suggesting. While keeping the refactors in the same PR sounds valid, the fact that this PR is comparatively small and easier to review makes it likely to be merged quickly. If that's the case, you can always rebase against master to incorporate the new changes.

I have updated the commit to make generic_autocomplete less redundant and more pythonic.

[kaustubh-nair]

@preetmishra Sorry I did not make myself more clear - One of the major reasons to avoid the refactors at different places was so that both of us don't waste our time working on the same thing. I have a similar commit here which is used for the footer PR

[preetmishra]

I have rebased the PR against the recent refactors that were pushed in #540. This should be independent and comparatively smaller to review now.

[neiljp]

@preetmishra Could you rebase? Also, the first commit is small now? I'll be interested to confirm how the code works for silent group mentions (which don't exist, by my understanding), and whether there are any strange cases where in vs startswith causes strange behavior

[preetmishra]

@neiljp Thanks for the review. I have rebased the changes and shelved the first commit (see #542 (comment)).

Re strange cases, I didn't come across any strange case while testing it manually or analyzing its logic.

[neiljp]

@preetmishra This certainly remains an improvement in terms of where we can perform autocomplete, but at minimum we might want to consider if we want to treat the following and similar examples more consistently:

• #@term -> #@*terminal* (# + @terminal)
• @#a -> no matches (should we expect @ + #announce ?)
• @@term -> no matches (should we expect @ + @terminal?)
• :#a -> :#announce (: + #announce)
• #:t -> no matches (should we expect#+ eg🎉`?)

[preetmishra]

@neiljp Thanks for catching that out! I have reworked the approach to find the last autocomplete prefix using a reverse lookup.

This should fix the inconsistencies that you reported (test cases are added) and should be more reliable.

[neiljp]

@preetmishra This looks good - and if we refactor the code later the tests should ensure things keep working 👍 Merging this now 🎉

We could maybe make the code simpler and without the specific case for the silent mention, but we have a limited subset of autocomplete so this is fine for now. I'm not sure if eg. a sorted rfind of prefixes might work?

[preetmishra]

Thanks for the merge! The sorted rfind of prefixes sounds appealing for refactoring. 🙌