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

[Do not merge] Add section support #139

Closed
wants to merge 32 commits into from
Closed

Conversation

feich-ms
Copy link
Contributor

@feich-ms feich-ms commented Sep 18, 2019

Add section support in lu file. Fixes microsoft/BotFramework-Composer#733

@feich-ms feich-ms marked this pull request as ready for review October 8, 2019 12:02
@feich-ms feich-ms changed the title [WIP]Add section support Add section support Oct 8, 2019
@feich-ms feich-ms requested review from vishwacsena, munozemilio, 1openwindow and Danieladu and removed request for 1openwindow October 8, 2019 13:25
Copy link
Contributor

@vishwacsena vishwacsena left a comment

Choose a reason for hiding this comment

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

We have a significant drop in coverage. Need additional tests. We should also review the back-compat issue with always expecting ## to be a sub-section.

packages/luis/src/parser/converters/luistoluconverter.js Outdated Show resolved Hide resolved
@feich-ms
Copy link
Contributor Author

feich-ms commented Oct 9, 2019

The test coverage drop mainly comes from newly added CRUD operations in luParser.js. Will add more tests soon.

@vishwacsena
Copy link
Contributor

We can review this once

  • Tests are reverting to keep the ## intent so we do not break back-compat
  • Remove luParser.js CRUD operations from this PR. Make a separate PR with this + tests.

@feich-ms please update this PR once the changes are in.

@feich-ms
Copy link
Contributor Author

feich-ms commented Oct 9, 2019

@vishwacsena

  1. I have reverted all the test cases that changed "##" to "#"..

  2. The CRUD changes are also removed.

  3. The changes in json files of test cases are just related to content order, not content itself

  4. if '> !# @enableSections = true' is not added in the header, below content will be identified as two intents(AA and BB, AA is an intent without utterances)
    #AA
    ##BB
    - hello world

    if '> !# @enableSections = true' is added in the header, above content will be identified as section(AA) and sub intent(BB)

  5. For the test coverage drop, I have no idea why it drops so much. I looked at the generated report, my newly added functions or contents are covered well by my newly added tests. so maybe @munozemilio can help to take a look? Notice that, all the files under src/parser/lufile/generated folder are auto generated by antlr, so maybe it should not be counted into the report?

@vishwacsena vishwacsena changed the title Add section support [Do not merge] Add section support Oct 9, 2019
@vishwacsena
Copy link
Contributor

@feich-ms We discussed this in BF-CLI review meeting today and decided to take these + other related changes for composer into a composer branch once we are locked down for ignite release (hopefully this week). So marking this as Do not merge. The only changes we want to take to master for now are related to Ignite deliverables.

@feich-ms
Copy link
Contributor Author

@vishwacsena, Ok, please let me know when the composer branch is ready. Thanks.

@feich-ms
Copy link
Contributor Author

Close this PR first, since all the composer related changes including section and section CRUD support are in this branch https://github.com/microsoft/botframework-cli/tree/composer now. When it is decided to go into the master, we can create a new PR then.

@feich-ms feich-ms closed this Oct 12, 2019
@feich-ms feich-ms deleted the feich/AddSectionSupport branch March 16, 2020 02:41
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.

4 participants