Skip to content

Conversation

EricDahlvang
Copy link
Member

Fixes #1347

While adding this, I realized on_installation_update should be named on_installation_update_activity... yet, we have released it as on_installation_update in 4.10.0 (although this is not likely to be used by any bots just yet, since the event is not sent from Teams yet). Any recommendation, should we update this event name to match the convention, or leave it?

@johnataylor
Copy link
Member

i can't really decide on the method name - i think we could leave it??? perhaps @tracyboehrer has an opinion.

@tracyboehrer
Copy link
Member

i can't really decide on the method name - i think we could leave it??? perhaps @tracyboehrer has an opinion.

I guess I would say "_activity" seems unneeded (though I've always wondered that). But specifically in this case, "on_installation_update" doesn't have that. So to be consistent, I think "on_installation_update_add" and "on_installation_update_remove" maybe better.

But... what did we do on the other repos?

@EricDahlvang
Copy link
Member Author

Javascript has the 'activity' appended to the method name of all three:
https://github.com/microsoft/botbuilder-js/blob/main/libraries/botbuilder-core/src/activityHandler.ts#L576

csharp has the 'activity' appended only to the initial event: https://github.com/microsoft/botbuilder-dotnet/blob/main/libraries/Microsoft.Bot.Builder/ActivityHandler.cs#L560 but not the add and remove counterparts. (Which is apparently the opposite of this python PR, just to confuse things further).

@EricDahlvang
Copy link
Member Author

@tracyboehrer I've removed '_activity' from the add and remove method names.

@tracyboehrer tracyboehrer merged commit 8970503 into main Oct 7, 2020
@tracyboehrer tracyboehrer deleted the eric/addInstallationUpdateSubEvents branch October 7, 2020 20:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

3 participants