Skip to content

Conversation

drub0y
Copy link
Contributor

@drub0y drub0y commented Jan 16, 2019

Fixes #1287

@drub0y
Copy link
Contributor Author

drub0y commented Jan 16, 2019

This effectively just copies the signature from the BotAdapter to the IAdapterIntegration abstraction.

@drub0y drub0y changed the title Adds ContinueConversationAsync to IAdapterIntegration Add ContinueConversationAsync to IAdapterIntegration Jan 16, 2019
@cleemullins
Copy link
Contributor

@drub0y , The build error says this:

tests\Microsoft.Bot.Builder.TestBot\InteceptorAdapter.cs(11,38): Error CS0535: 'InteceptorAdapter' does not implement interface member 'IAdapterIntegration.ContinueConversationAsync(string, ConversationReference, BotCallbackHandler, CancellationToken)'
tests\Microsoft.Bot.Builder.TestBot.WebApi\InteceptorAdapter.cs(11,38): Error CS0535: 'InteceptorAdapter' does not implement interface member 'IAdapterIntegration.ContinueConversationAsync(string, ConversationReference, BotCallbackHandler, CancellationToken)'
Process 'msbuild.exe' exited with code '1'.

(Note: Amusingly, InteceptorAdapter looks to be spelled wrong. If you're in there anyway, could you fix the spelling error?)

@cleemullins
Copy link
Contributor

I wonder - does adding a method to an interface definition count as a breaking change? It clearly is (as seen by the broken build), but I'm not sure what to make of that.

This seems like a class that's deep enough that nobody would be broken by taking the change.

@drub0y
Copy link
Contributor Author

drub0y commented Jan 17, 2019

So it's really strange that I didn't hit this locally, I will run that to grounds and get whatever fix is necessary in.

The test projects contain an `IntercepterAdapter` class for demonstration purposes and those needed to implement the new `IAdapterIntegration::ContinueConversationAysnc` method.
@drub0y
Copy link
Contributor Author

drub0y commented Jan 17, 2019

So, I really don't know what I did that missed this because I usually just build the whole solution, but clearly I must have built some subset. The problem was sample IAdapterIntegration classes in the Test apps in the solution, called IntercepterAdapter, that needed to implement the new method of the interface. Should be fixed, waiting on server build.

@coveralls
Copy link
Collaborator

Pull Request Test Coverage Report for Build 47177

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • 1 unchanged line in 1 file lost coverage.
  • Overall coverage decreased (-0.01%) to 55.773%

Files with Coverage Reduction New Missed Lines %
/libraries/Microsoft.Bot.Builder/TranscriptLoggerMiddleware.cs 1 97.67%
Totals Coverage Status
Change from base Build 46954: -0.01%
Covered Lines: 5497
Relevant Lines: 9856

💛 - Coveralls

@drub0y
Copy link
Contributor Author

drub0y commented Jan 17, 2019

I wonder - does adding a method to an interface definition count as a breaking change? It clearly is (as seen by the broken build), but I'm not sure what to make of that.

Just to keep the discussion up to date here, @cleemullins had an offline chat and, yes, this is a breaking change because people who may have implemented the interface would be broken and need to implement the new method. In your own app this might not be a big deal, but if you took a dependency on another package that implemented a class with this interface it would be a bigger issue. You would have to wait until that package provided a compatible version before you could upgrade the BBSDK.

The decision will need to be made if this is the exact approach taken to keep things simple because there probably aren't many people out there who have implemented this interface or if we need to consider a different change (e.g. the old IAdapterIntegration2 approach). I can't imagine anyone out there has implemented this interface themselves yet and then it should just be called out as a minor breaking change in the release notes for the rare case that someone did. My only other thought is: strict adherence to semver rules can cripple progress, so it really needs to be weighed and decided upon on a case by case basis.

Finally, this is the reason why the C# team is working on the default interface implementations feature.

Copy link
Contributor

@cleemullins cleemullins left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Signed off during phone PR review.

@cleemullins cleemullins merged commit 50912ad into master Jan 23, 2019
@cleemullins cleemullins deleted the drmarsh/iadapterintegration-contiuneconversationasync branch January 23, 2019 18:45
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.

3 participants