Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add installation update sub events to ActivityHandler #1368

Merged
merged 2 commits into from
Oct 7, 2020

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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

InstallationUpdate should have actions "add" and "remove"
3 participants