Fix #862 - Sorts Help Commands by Script

[cycomachead]

This changes @commands from [] → {} which keeps track of each script
as they are loaded. Then helpCommands returns an array after calling
sort() on the commands of each script, rather than all scripts at once.

Note that this means that the command hubot help <TERM> will show
commands sorted by script still, since the filtering is done in the help
script. A simple solution would be to have the help scripts call sort()
after filtering, but this, of course, wouldn't be applied to existing
installations.

[technicalpickles]

How would this effect existing scripts that access robot.commands? I'm worried there are scripts out in the wild using this, and this would break compatibility.

Maybe could save this to a different object instead?

[cycomachead]

How would this effect existing scripts that access robot.commands? I'm worried there are scripts out in the wild using this, and this would break compatibility.

True, since commands now returns an object it could break if accessed directly. However, that was always the same functionality as helpCommands(), so I don't think there would be a reason to access it directly. I'm not sure if any scripts do access robot.commands but I see your concerns.

If two objects is ok, then that's easy to. What would be a good name? commandsByScript?

[cycomachead]

Also, should I do anything about the Travis error on 0.8.x? It looks like other builds have been failing recently, but I wasn't positive if it was the same...

[michaelansel]

We've seen transient failures for 0.8 in the past. @technicalpickles can you fire it off again to see it the error clears?

For backwards compatibility, I don't think we should support usage of internal variables in Robot. We should definitely lean on the side of maintaining backwards compatibility, but this feels like we are stretching a bit too far. Given that the "proper" method of retrieving the command list is via the method, any breakage should be easy to resolve by using the standard interface.

On another note, I would love to see tests added here for helpCommands and parseHelp. I recognize parseHelp is non-trivial, but I think we should be increasing test coverage here as quick as possible. For examples, take a look at robot_test.coffee or see #903.

[technicalpickles]

We've seen transient failures for 0.8 in the past. @technicalpickles can you fire it off again to see it the error clears?

They've been failing pretty consistently, so it's probably time to dig in and/or contact Travis about it.

For backwards compatibility, I don't think we should support usage of internal variables in Robot. We should definitely lean on the side of maintaining backwards compatibility, but this feels like we are stretching a bit too far. Given that the "proper" method of retrieving the command list is via the method, any breakage should be easy to resolve by using the standard interface.

I can appreciate the sentiment, it's just hard to take a hard stance though, when there's so much stuff out in the wild, and there's not necessarily an API for doing things without touching internal variables.

I did a review of our own hubot, and we have one script that dynamically manipulates the robot.commands object to add more help commands. Basically, it's our shell to hubot bridge, and it dynamically adds robot.respond calls and then adds stuff to robot.commands so they can be discovered with /help. That's expected to be an array, so would be a breaking change for us.

I think that points to needing a function for adding for adding to the list of known commands, which hubot could use internally, as scripts using it externally.

[cycomachead]

@technicalpickles @michaelansel
No rush on this, but anything I should do on my end about getting this merged? IMO, I agree with Michael about the support, but I'll defer to your experience about what would be the best way to continue.

I know it's been crazy lately, so thanks for everything you've all be been doing to make Hubot an awesome tool (and friend 😉 )!

👏 👏

[technicalpickles]

The test is still failing which is weird.

I'm still really hesitant to merge this because of API concerns I've mentioned. I know I'll have to make changes to our hubot for this upgrade, which says it's a breaking change to me. I would feel better if @commands stayed an array.

I could imagine a parrell object to it though. @commandsByName? And then some helpers for registering commands, and make sure it gets consistently added to both.

[technicalpickles]

@cycomachead we had a pretty big PR drop (#803), so this isn't merging cleanly any more.

I'd also like to point out something in the similar realm, #1025 . That'd expose more of the script documentation, and the hubot-advanced-help might provide a similar user experience you were suggesting.

[bkeepers]

Closing since this is quite stale and there are merge conflicts. Thanks for taking time to contribute and sorry we weren't able to get this merged.