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.

Sign up for GitHub

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

Jump to bottom

Migrate expression namespace to AdaptiveExpressions #3375

Merged
merged 2 commits into from
Feb 12, 2020
Merged

Migrate expression namespace to AdaptiveExpressions #3375

merged 2 commits into from
Feb 12, 2020

Conversation

Danieladu
Copy link
Collaborator

Migrate expression namespace from Microsoft.Bot.Expressions to AdaptiveExpressions,
ref: #3249

@Danieladu Danieladu added the LG label Feb 11, 2020
@Danieladu Danieladu changed the title Migrate expression namespace AdaptiveExpressions Migrate expression namespace to AdaptiveExpressions Feb 11, 2020
@Danieladu Danieladu removed the request for review from matthidinger February 11, 2020 01:57
tomlm
tomlm reviewed Feb 11, 2020
View reviewed changes
libraries/AdaptiveExpressions/AdaptiveExpressions.csproj
@@ -20,7 +20,7 @@

<PropertyGroup>
<TargetFramework>netstandard2.0</TargetFramework>
<PackageId>Microsoft.Bot.Expressions</PackageId>
<PackageId>AdaptiveExpressions</PackageId>
Copy link
Contributor

Choose a reason for hiding this comment

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

This needs to be Adaptive.Expressions with a period. Adaptive already has a number of a packages which use the dot nomenclature
Adaptive.WPF
Adaptive.Xaml
etc.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmmm..Actually, they are AdaptiveCards.WPF, AdaptiveCards.Xaml etc.
Do we want Adaptive or AdaptiveCards?

In reply to: 377424557 [](ancestors = 377424557)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@vishwacsena maybe we should confirm this?

Copy link
Contributor

Choose a reason for hiding this comment

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

yes, let @vishwacsena to confirm, i thought we should call this AdapativeExpression

tomlm
tomlm reviewed Feb 11, 2020
View reviewed changes
libraries/Microsoft.Bot.Builder.Dialogs.Adaptive/Actions/EmitEvent.cs
@@ -5,7 +5,7 @@
using System.Runtime.CompilerServices;
using System.Threading;
using System.Threading.Tasks;
using Microsoft.Bot.Expressions.Properties;
using AdaptiveExpressions.Properties;
Copy link
Contributor

Choose a reason for hiding this comment

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

AdaptiveExpressions [](start = 6, length = 19)

Likewise, Adaptive.Expressions

Copy link
Contributor

Choose a reason for hiding this comment

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

Was reviewed in last adaptive cards working group meeting. AdaptiveExpressions was were we landed. @matthidinger.

Copy link
Contributor

Choose a reason for hiding this comment

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

Haven't seen this, so we are landed on AdapativeExpresion, I added an approve on this

tomlm
tomlm previously requested changes Feb 11, 2020
View reviewed changes
Copy link
Contributor

@tomlm tomlm left a comment

Choose a reason for hiding this comment

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

🕐

@Danieladu Danieladu requested a review from tomlm February 11, 2020 03:46
@Danieladu Danieladu added the P0 Must Fix. Release-blocker label Feb 12, 2020
@coveralls
Copy link
Collaborator

Pull Request Test Coverage Report for Build 105558

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • 3 unchanged lines in 1 file lost coverage.
  • Overall coverage remained the same at 79.372%
Files with Coverage Reduction New Missed Lines %
/libraries/Microsoft.Bot.Builder.LanguageGeneration/Analyzer.cs 3 96.55%
Totals Coverage Status
Change from base Build 105552: 0.0%
Covered Lines: 10701
Relevant Lines: 13482
💛 - Coveralls

@fuselabs
Copy link
Collaborator

✔️ No Binary Compatibility issues for Microsoft.Bot.Builder.dll compared against version 4.6.3
✔️ No Binary Compatibility issues for Microsoft.Bot.Builder.AI.Luis.dll compared against version 4.6.3
✔️ No Binary Compatibility issues for Microsoft.Bot.Builder.AI.QnA.dll compared against version 4.6.3
✔️ No Binary Compatibility issues for Microsoft.Bot.Builder.ApplicationInsights.dll compared against version 4.6.3
✔️ No Binary Compatibility issues for Microsoft.Bot.Builder.Azure.dll compared against version 4.6.3
✔️ No Binary Compatibility issues for Microsoft.Bot.Builder.Dialogs.dll compared against version 4.6.3
✔️ No Binary Compatibility issues for Microsoft.Bot.Builder.Integration.ApplicationInsights.Core.dll compared against version 4.6.3
✔️ No Binary Compatibility issues for Microsoft.Bot.Builder.Integration.AspNet.Core.dll compared against version 4.6.3
✔️ No Binary Compatibility issues for Microsoft.Bot.Builder.TemplateManager.dll compared against version 4.6.3
✔️ No Binary Compatibility issues for Microsoft.Bot.Builder.Testing.dll compared against version 4.6.3
✔️ No Binary Compatibility issues for Microsoft.Bot.Configuration.dll compared against version 4.6.3
✔️ No Binary Compatibility issues for Microsoft.Bot.Connector.dll compared against version 4.6.3
✔️ No Binary Compatibility issues for Microsoft.Bot.Schema.dll compared against version 4.6.3
✔️ No Binary Compatibility issues for Microsoft.Bot.Streaming.dll compared against version 4.6.3

@Danieladu Danieladu merged commit 36e0b13 into master Feb 12, 2020
This was referenced Feb 12, 2020
Closed
Closed
Closed
@Danieladu Danieladu deleted the hond/new-namespace branch March 13, 2020 01:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@boydc2014 boydc2014 boydc2014 approved these changes

@chrimc62 chrimc62 Awaiting requested review from chrimc62

@vishwacsena vishwacsena Awaiting requested review from vishwacsena

@tomlm tomlm Awaiting requested review from tomlm

Assignees
No one assigned
Labels
P0 Must Fix. Release-blocker
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@Danieladu @coveralls @fuselabs @boydc2014 @tomlm @vishwacsena