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

[Lubuild] fix multiple bugs: case sensitive issue, default version issue and exception not throw out issue - #576

Merged
Fei Chen (feich-ms) merged 30 commits into
masterfrom
feich/fixLubuildBug
Feb 27, 2020
Merged

[Lubuild] fix multiple bugs: case sensitive issue, default version issue and exception not throw out issue#576
Fei Chen (feich-ms) merged 30 commits into
masterfrom
feich/fixLubuildBug

Conversation

@feich-ms

@feich-ms Fei Chen (feich-ms) commented Feb 17, 2020 •
edited
Loading

Copy link
Copy Markdown
Contributor

Fix several bugs:

  1. As known to us, values of utterances/patterns are automatically converted to lower case in luis portal even though typed words are upper case. When comparing app json from lu file with the one from remote existing luis app, we should to ignore cases. To achieve this, I converted the objects to string, then to lower case so that the comparison works insensitively.
  2. Some properties in our luis definition don't align with the object pulled from luis portal. Fixed in this PR
  3. Default version 0.1 was included by previous PR when no version is specified which caused some bugs when updating version. In this PR, version from lu will count only if it's value is over than existing app. For example, if the app with the same app id in luis portal is version 0.1, and the version from the lu file is 0.3, 0.3 > 0.1, then 0.3 will count, otherwise, 0.1 will auto increase by 0.1 to 0.2
  4. Add test cases for above bug fixes
  5. Add test cases for luis:build refinements #548
  6. Fix bug Exceptions are not returned properly in luis:build #582

@feich-ms Fei Chen (feich-ms) changed the title [Lubuild] fix case sensitive issue when comparing luis applications json [Lubuild] fix case sensitive issue when comparing luis applications json to update Feb 17, 2020
vishwacsena
Vishwac Sena Kannan (vishwacsena) suggested changes Feb 17, 2020
View reviewed changes

@vishwacsena Vishwac Sena Kannan (vishwacsena) left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Can you please add tests as well?

@feich-ms

Fei Chen (feich-ms) commented Feb 17, 2020

Copy link
Copy Markdown
Contributor Author

Vishwac Sena Kannan (@vishwacsena), sure, will add tests soon.

@feich-ms

Fei Chen (feich-ms) commented Feb 18, 2020

Copy link
Copy Markdown
Contributor Author

Tests added Vishwac Sena Kannan (@vishwacsena)

vishwacsena
Vishwac Sena Kannan (vishwacsena) approved these changes Feb 18, 2020
View reviewed changes

@vishwacsena Vishwac Sena Kannan (vishwacsena) left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Emilio Munoz (@munozemilio) it might make sense to expose compareApps as a public method on LUIS?

munozemilio
Emilio Munoz (munozemilio) reviewed Feb 19, 2020
View reviewed changes
Comment thread packages/lu/test/commands/luis/build.test.ts
munozemilio
Emilio Munoz (munozemilio) suggested changes Feb 19, 2020
View reviewed changes

@munozemilio Emilio Munoz (munozemilio) left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

CIL

@feich-ms

Fei Chen (feich-ms) commented Feb 19, 2020

Copy link
Copy Markdown
Contributor Author

Emilio Munoz (@munozemilio), comments resolved. Also added test cases for luConfig support of #548. Fixed bug of #582 as well.

@feich-ms Fei Chen (feich-ms) changed the title [Lubuild] fix case sensitive issue when comparing luis applications json to update [Lubuild] fix multiple bugs: case sensitive issue, default version issue and exception not throw out issue Feb 19, 2020
@feich-ms Fei Chen (feich-ms) mentioned this pull request Feb 19, 2020
Closed
munozemilio
Emilio Munoz (munozemilio) reviewed Feb 19, 2020
View reviewed changes
Comment thread packages/lu/src/parser/lubuild/builder.ts Outdated
munozemilio
Emilio Munoz (munozemilio) reviewed Feb 19, 2020
View reviewed changes
Comment thread packages/luis/src/commands/luis/build.ts
munozemilio
Emilio Munoz (munozemilio) reviewed Feb 19, 2020
View reviewed changes

@munozemilio Emilio Munoz (munozemilio) left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Some comments in line

@feich-ms

Fei Chen (feich-ms) commented Feb 20, 2020

Copy link
Copy Markdown
Contributor Author

Comments resolved. Thanks Emilio Munoz (@munozemilio)

@vishwacsena

Vishwac Sena Kannan (vishwacsena) commented Feb 21, 2020

Copy link
Copy Markdown
Contributor

Fei Chen (@feich-ms) I tried this E2E and pushed up two sets of deltas up to your branch. I'm not familiar with nock and some of the lubuild tests are failing. Can you take a look and address?

@vishwacsena

Vishwac Sena Kannan (vishwacsena) commented Feb 21, 2020

Copy link
Copy Markdown
Contributor

Emilio Munoz (@munozemilio) except for addressing tests, it would be good if you get a review as well. Thanks!

munozemilio
Emilio Munoz (munozemilio) reviewed Feb 21, 2020
View reviewed changes
Comment thread packages/lu/src/parser/lubuild/builder.ts Outdated
munozemilio
Emilio Munoz (munozemilio) reviewed Feb 21, 2020
View reviewed changes
Comment thread packages/lu/src/parser/lubuild/core.ts Outdated
munozemilio
Emilio Munoz (munozemilio) reviewed Feb 21, 2020
View reviewed changes
Comment thread packages/lu/src/parser/luis/luis.js Outdated
munozemilio
Emilio Munoz (munozemilio) suggested changes Feb 21, 2020
View reviewed changes

@munozemilio Emilio Munoz (munozemilio) left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

CIL

@feich-ms

Fei Chen (feich-ms) commented Feb 24, 2020 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Emilio Munoz (@munozemilio), tests failures are fixed. Please help to approve. Thanks.

The merge is blocked by 'Request to change'. Could you approve if there are no other issues.

@feich-ms

Fei Chen (feich-ms) commented Feb 27, 2020 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Emilio Munoz (@munozemilio), I added more modules in index.js and composerindex.js in the latest commit. My intention is to expose modules that composer client/broswer consumes in composerindex.js and other modules in index.js. Parser and SectionHandler is consumed by composer client and luBuild is consumed by composer server. Please let me know if this is not the reasonable way. Thanks.

munozemilio
Emilio Munoz (munozemilio) reviewed Feb 27, 2020
View reviewed changes
Comment thread packages/lu/src/parser/index.js Outdated
munozemilio
Emilio Munoz (munozemilio) approved these changes Feb 27, 2020
View reviewed changes

@munozemilio Emilio Munoz (munozemilio) left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Approved with a comment

@feich-ms
Fei Chen (feich-ms) merged commit 49f1871 into master Feb 27, 2020
@feich-ms
Fei Chen (feich-ms) deleted the feich/fixLubuildBug branch February 27, 2020 05:11
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@Danieladu Hongyang Du (hond) (Danieladu) Awaiting requested review from Danieladu
@boydc2014 Dong Lei (boydc2014) Awaiting requested review from boydc2014
2 more reviewers
@vishwacsena Vishwac Sena Kannan (vishwacsena) vishwacsena approved these changes
@munozemilio Emilio Munoz (munozemilio) munozemilio approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@feich-ms @vishwacsena @munozemilio