Skip to content
This repository was archived by the owner on Jan 5, 2026. It is now read-only.

PR for New sample Codes#2990

Closed
Trinetra-MSFT wants to merge 1 commit intomicrosoft:mainfrom
Trinetra-MSFT:v-trkum-SampleCodes
Closed

PR for New sample Codes#2990
Trinetra-MSFT wants to merge 1 commit intomicrosoft:mainfrom
Trinetra-MSFT:v-trkum-SampleCodes

Conversation

@Trinetra-MSFT
Copy link
Contributor

@Trinetra-MSFT Trinetra-MSFT commented Dec 16, 2020

justinTimeInstallationBot
InlineImageDownload- Bot
SubscribeToBotConversationEvents

Fixes #

Proposed Changes

  1. These are new sample codes for JustinTimeInstallation for Bot, InlineImageDownload and SubscribeToConversationEvents

Testing

InlineImageDownload- Bot
SubscribeToBotConversationEvents
@Trinetra-MSFT Trinetra-MSFT requested a review from a team as a code owner December 16, 2020 04:49
@ghost
Copy link

ghost commented Dec 16, 2020

CLA assistant check
All CLA requirements met.

@Wajeed-msft Wajeed-msft requested a review from clearab December 17, 2020 04:18
@clearab
Copy link
Contributor

clearab commented Dec 17, 2020

@Wajeed-msft & @Trinetra-MSFT - I love that you guys want to improve these samples, and I really don't want to discourage that at all. But can you please check with us before adding completely new ones? Any new sample here creates an ongoing maintenance cost, and we need to make sure everything rationalizes correctly. Can you schedule some time for us to talk through this in January, and we can get some Issues created to track this work and make sure it get's prioritized (and the changes get ported into all the other languages)?

Based on my quick look through this:

  • the events sample should be combined with the existing sample 57
  • the just in time install should be added to sample 51
    • get members is an expensive call to test if you're installed, should probably use getTeamDetails instead (fairly certain this call works in all contexts, despite the name)
    • The nested catch is a little awkward, hopefully there's a way to clean that up?
  • Image download should probably also be added to sample 57, with the logic for getting the image abstracted into its own function. The try/catch here is also a little off, is there a better way?

@johnataylor & @EricDahlvang does the feedback above look correct to y'all? I'm a little worried about sample 57 getting overly large.

@EricDahlvang
Copy link
Member

'inline image download' might be better combined with '56.teams-file-upload' and changed to '56.teams-files' or something.? I think we demonstrate this same principal in the '15.handling-attachments' sample.

@clearab
Copy link
Contributor

clearab commented Dec 17, 2020

Looking at 15, I think that is probably the right one to combine with.

@Trinetra-MSFT
Copy link
Contributor Author

Thanks @clearab , @EricDahlvang , I will look into your review comments and try to accommodate these samples in existing ones.

@clearab
Copy link
Contributor

clearab commented Dec 28, 2020

I'm going to go ahead and close this PR - I'll schedule some time for us to talk in early January to agree on how this functionality should get added.

@clearab clearab closed this Dec 28, 2020
@Trinetra-MSFT
Copy link
Contributor Author

@clearab , I worked on your review comments and merged code in existing samples, We will discuss these changes in Jan and understand your inputs on these so we can raise PR again. Thanks

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants