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

Dynamic Form Docs Section Update #10734

Merged
merged 4 commits into from
Jan 25, 2022
Merged

Dynamic Form Docs Section Update #10734

merged 4 commits into from
Jan 25, 2022

Conversation

ancalita
Copy link
Member

@ancalita ancalita commented Jan 25, 2022

Proposed changes:

Status (please check what you already did):

  • added some tests for the functionality
  • updated the documentation
  • updated the changelog (please check changelog for instructions)
  • reformat files using black (please check Readme for instructions)

@ancalita ancalita marked this pull request as ready for review January 25, 2022 09:30
@ancalita ancalita requested a review from indam23 January 25, 2022 09:30
Copy link
Contributor

@indam23 indam23 left a comment

Choose a reason for hiding this comment

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

Just some small word order things =)

@@ -388,6 +388,35 @@ class ValidateRestaurantForm(FormValidationAction):
return additional_slots + domain_slots
```

If conversely, you'd prefer to remove a slot from the form's `required_slots` defined in the domain file in certain conditions,
we recommend to copy the `domain_slots` over to a new variable and applying changes to that new variable instead of modifying directly
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
we recommend to copy the `domain_slots` over to a new variable and applying changes to that new variable instead of modifying directly
you can copy the `domain_slots` over to a new variable and apply changes to that new variable instead of directly modifying

Copy link
Contributor

Choose a reason for hiding this comment

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

will anything bad happen if they do modify it directly or is it just for clarity?

Copy link
Member Author

Choose a reason for hiding this comment

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

They won't get the behaviour they want (not requesting the removed slot), because Rasa will instead use the list of required slots defined in the domain, I explained how this occurs in the comment here.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah I see thanks!
In that case we should be stronger in the wording e.g.

you should copy the `domain_slots` over to a new variable and apply changes to that new variable instead of directly modifying `domain_slots`. Directly modifying `domain_slots` can cause unexpected behaviour.

docs/docs/forms.mdx Outdated Show resolved Hide resolved
@ancalita ancalita requested a review from indam23 January 25, 2022 10:36
Copy link
Contributor

@indam23 indam23 left a comment

Choose a reason for hiding this comment

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

🚀

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.

Form next requested slot incorrectly asked in Rasa 3.0, although it was removed from required slots
2 participants