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

add #10137 changes to domain merging #10589

Merged
merged 7 commits into from Jan 4, 2022
Merged

add #10137 changes to domain merging #10589

merged 7 commits into from Jan 4, 2022

Conversation

carlad
Copy link
Contributor

@carlad carlad commented Dec 27, 2021

Proposed changes:
closes #10570

Status (please check what you already did):

  • added some tests for the functionality
  • updated the documentation
  • updated the changelog (please check changelog for instructions)

@carlad carlad requested a review from a team as a code owner December 27, 2021 17:09
@carlad carlad requested review from wochinge, alwx, jupyterjazz and ePreda and removed request for a team December 27, 2021 17:09
rasa/shared/core/domain.py Show resolved Hide resolved
@m-vdb m-vdb removed the request for review from alwx January 3, 2022 10:23
@wochinge wochinge requested review from joejuzl and removed request for wochinge January 3, 2022 15:37
@carlad carlad requested a review from ePreda January 3, 2022 15:45
@carlad
Copy link
Contributor Author

carlad commented Jan 3, 2022

@ePreda I realised that there was some logic in the Domain class to detect duplication that I had accidentally removed. It was added to address a cli issue #9351 which I wasn't aware of.

def merge_lists(list1: List[Any], list2: List[Any]) -> List[Any]:
return sorted(list(set(list1 + list2)))
This is used when multiple domain yml files are configured in a single
directory. Unlike the merge method above, which merges Domain objects by
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason we can't use this implementation for both?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@joejuzl So this PR is to port the changes from 2.8.x into main, which we decided to implement in this way as we were planning to rethink how we handle the domain.

But as we wanted to ensure that main also handled multiple domain files, we thought it would be better in the short-term to implement this solution...

@github-actions
Copy link
Contributor

github-actions bot commented Jan 3, 2022

🚀 A preview of the docs have been deployed at the following URL: https://10589--rasahq-docs-rasa-v2.netlify.app/docs/rasa

@carlad carlad requested a review from tayfun January 4, 2022 09:15
Copy link
Contributor

@tayfun tayfun left a comment

Choose a reason for hiding this comment

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

Approving as this is already in 2.8.x. We should come back to this and find a more elegant solution and remove duplication as well like @joejuzl has mentioned in the comment, specifically around similar merge domain methods.

@carlad carlad merged commit 0c5241b into main Jan 4, 2022
@carlad carlad deleted the 10570-10137-into-main branch January 4, 2022 10:42
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.

Apply #10137 domain merging changes to main
4 participants