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

feat: port several runtime changes #3446

Merged
merged 4 commits into from
Mar 25, 2021
Merged

feat: port several runtime changes #3446

merged 4 commits into from
Mar 25, 2021

Conversation

joshgummersall
Copy link
Contributor

Josh Gummersall added 2 commits March 24, 2021 18:35
This ports the ConfigurationResourceProvider from .NET. In addition, it makes some changes to Configuration handling
throughout the runtime.

The .NET configuration interface is not async, so let's match that. In particular, anything that operates on a
Configuration instance can, itself, resolve any async operations before or after mutations.

This will provide better interop with existing Adaptive classes, the prime example being the Resource Explorer.
Unfortunately, most of that code was written in a way that is not terribly async friendly. It's easier to push async
operations outside the Configuration interface, so that's what we do.
@joshgummersall joshgummersall requested a review from a team as a code owner March 25, 2021 17:43
@joshgummersall joshgummersall changed the title feat: port several runtime changes= feat: port several runtime changes Mar 25, 2021
@coveralls
Copy link

coveralls commented Mar 25, 2021

Pull Request Test Coverage Report for Build 687579641

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.006%) to 85.028%

Totals Coverage Status
Change from base Build 684901568: 0.006%
Covered Lines: 18818
Relevant Lines: 21075

💛 - Coveralls

Copy link
Member

@stevengum stevengum left a comment

Choose a reason for hiding this comment

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

Had one bit of feedback around the new static members, but otherwise it looks good. Thanks for adding the comment around the async-to-sync changes in the PR:

This ports the ConfigurationResourceProvider from .NET. In addition, it makes some changes to Configuration handling
throughout the runtime.

The .NET configuration interface is not async, so let's match that. In particular, anything that operates on a
Configuration instance can, itself, resolve any async operations before or after mutations.

This will provide better interop with existing Adaptive classes, the prime example being the Resource Explorer.
Unfortunately, most of that code was written in a way that is not terribly async friendly. It's easier to push async
operations outside the Configuration interface, so that's what we do.

via commit 79782de

@joshgummersall joshgummersall force-pushed the jpg/port-runtime-changes branch from 00b944d to ebd1741 Compare March 25, 2021 18:41
Copy link
Member

@stevengum stevengum left a comment

Choose a reason for hiding this comment

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

LGTM!

@stevengum stevengum merged commit 9017aea into main Mar 25, 2021
@stevengum stevengum deleted the jpg/port-runtime-changes branch March 25, 2021 19:46
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.

port: Update CoreBotAdapter to derive from CloudAdapter (#5353)
3 participants