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

Schema cleanup and validation #3758

Merged
merged 21 commits into from
Apr 20, 2020
Merged

Schema cleanup and validation #3758

merged 21 commits into from
Apr 20, 2020

Conversation

tomlm
Copy link
Contributor

@tomlm tomlm commented Apr 17, 2020

  • moved dialogManager into dialogs assembly
  • cleanup input schema and comments
  • Add unit tests for validating dialogs/schema (!!!)
  • fix a bunch of schema errors
  • fix a bunch of dialog errors
  • move compnent schema to using botframework-sdk/component.schema
  • get rid of sdk.schema, schema comes from SDK assemblies not from sdk.schema
  • create tests.schema which is active schema for unit tests
  • create testbot.schema which is active schema for testbot

Tom Laird-McConnell added 6 commits April 10, 2020 12:38
move dialogmanager into dialogs assembly
4c8db72
rename DeclarativeType constants => Kind
66893fc
clean up input schema and comments
f82e332 
renamed Ask.ExpectedOperation to Ask.DefaultOperation
merge master
bfd80ad
* Add unit tests for validating dialogs
9fddf2e 
* fix a bunch of schema errors
* fix a bunch of dialog errors
* move compnent schema to using botframework-sdk/component.schema
* get rid of sdk.schema, schema comes from SDK assemblies not from sdk.schema
* create tests.schema which is active schema for unit tests
* create testbot.schema which is active schema for testbot
Merge branch 'master' into tomlm/miscCleanup
5ede25a
@tomlm tomlm changed the title Tomlm/misc cleanup Schema cleanup and validation Apr 17, 2020
@tomlm tomlm linked an issue Apr 17, 2020 that may be closed by this pull request
Prompt output format error & enhancement #3700
Closed
7 tasks
johnataylor
johnataylor approved these changes Apr 17, 2020
View reviewed changes
tests/Microsoft.Bot.Builder.Dialogs.Adaptive.Tests/GeneratorTests.cs
@@ -26,7 +26,7 @@ public static void ClassInitialize(TestContext context)
{
ResourceExplorer = new ResourceExplorer()
Copy link
Member

@johnataylor johnataylor Apr 17, 2020 •
edited by tomlm

Choose a reason for hiding this comment

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

did you intend to remove this file? #Closed

tests/Microsoft.Bot.Builder.Dialogs.Declarative.Tests/SchemaMergeTests.cs Outdated
// string output = process.StandardOutput.ReadToEnd();

var process = Process.Start(startInfo);
string error = process.StandardError.ReadToEnd();
Copy link
Member

Choose a reason for hiding this comment

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

var

tests/Microsoft.Bot.Builder.Dialogs.Declarative.Tests/SchemaMergeTests.cs
[DynamicData(nameof(Dialogs))]
public async Task TestDialogResourcesAreValidForSchema(IResource resource)
{
FileResource fileResource = resource as FileResource;
Copy link
Member

Choose a reason for hiding this comment

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

var

tests/Microsoft.Bot.Builder.TestBot.Json/CustomSteps/MultiplyDialog.cs Outdated

public override Task<DialogTurnResult> BeginDialogAsync(DialogContext dc, object options = null, CancellationToken cancellationToken = default(CancellationToken))
{
var (arg1, err1) = Expression.Parse(Arg1).TryEvaluate(dc.State);
var (arg2, err2) = Expression.Parse(Arg2).TryEvaluate(dc.State);
var arg1 = Arg1.GetValue(dc.State);
Copy link
Member

Choose a reason for hiding this comment

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

whitespace picked up by tool

This was linked to issues Apr 17, 2020
null filled in value field after add/remove the content in it. #3716
Closed
Add Serialization Tests to builds for Adaptive Dialogs #3687
Closed
This was linked to issues Apr 18, 2020
display default value for certain variables #3636
Closed
if condition default value is empty #3634
Closed
add normalized defaults to expressionProperty
76e44f1 
* boolProperty => false
* numberProperty = 0
* integerProperty =0
Ordered so default form is first OneOf() array
* BoolProperty => default is bool
* NumberProperty => default is number
* Condition => default is string with pattern
* StringExpression => default is string
etc.
* add checks for $schema exists and is correct.
* fix up all $schema files.
@tomlm tomlm linked an issue Apr 18, 2020 that may be closed by this pull request
[Adaptive] ChoiceInput and ConfirmInput do no have consistent compoent schema #3585
Closed
@tomlm tomlm linked an issue Apr 18, 2020 that may be closed by this pull request
Request the 'ChoiceInput' form provides an extra field 'choice.description' over 'choice.value' #3628
Closed
@chrimc62
Copy link
Contributor

Two major things:

  1. I don't think your regex patterns are right?
  2. I'm confused whether we need id and $id. Should I change $id to id and remove them globally?

chrimc62
chrimc62 suggested changes Apr 19, 2020
View reviewed changes
Copy link
Contributor

@chrimc62 chrimc62 left a comment

Choose a reason for hiding this comment

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

🕐

@tomlm
Copy link
Contributor Author

tomlm commented Apr 19, 2020 

    "id": {

resource.id vs dialog.id

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

Refers to: libraries/Microsoft.Bot.Builder.Dialogs.Adaptive/Schemas/Actions/Microsoft.BeginDialog.schema:8 in c276987. [](commit_id = c276987, deletion_comment = False)

@tomlm tomlm requested a review from chrimc62 April 19, 2020 21:13
chrimc62
chrimc62 approved these changes Apr 20, 2020
View reviewed changes
Copy link
Contributor

@chrimc62 chrimc62 left a comment

Choose a reason for hiding this comment

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

:shipit:

@cleemullins
Copy link
Contributor

I suggest we close this PR and break it into smaller PRs that are possible to review. The size of this PR, and the mixture of work, makes it really hard to tell what this is.

@tomlm tomlm merged commit 92242ea into master Apr 20, 2020
@tomlm tomlm deleted the tomlm/miscCleanup branch April 20, 2020 20:25
This was referenced Apr 20, 2020
Closed
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cleemullins cleemullins cleemullins left review comments

@johnataylor johnataylor johnataylor approved these changes

@chrimc62 chrimc62 chrimc62 approved these changes

@carlosscastro carlosscastro carlosscastro approved these changes

@boydc2014 boydc2014 Awaiting requested review from boydc2014

@gabog gabog Awaiting requested review from gabog

@mrivera-ms mrivera-ms Awaiting requested review from mrivera-ms

@munozemilio munozemilio Awaiting requested review from munozemilio

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
5 participants
@tomlm @chrimc62 @cleemullins @johnataylor @carlosscastro