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

Rename FormValidation event to something "loop" related #6463

Closed
2 tasks done
wochinge opened this issue Aug 21, 2020 · 25 comments
Closed
2 tasks done

Rename FormValidation event to something "loop" related #6463

wochinge opened this issue Aug 21, 2020 · 25 comments
Assignees
Labels
area:rasa-oss 🎡 Anything related to the open source Rasa framework type:enhancement ✨ Additions of new features or changes to existing ones, should be doable in a single PR

Comments

@wochinge
Copy link
Contributor

wochinge commented Aug 21, 2020

Follow up from #6409 and PR #6439 .

Same as we re-named the Form event, we should also re-name the FormValidation event.

Todos

@Ghostvv @akelad Please fire away with naming suggestions. As we don't validate after a Loop was re-called after an unhappy path, it makes sense to call it something like LoopExecutionAfterUnhappyPath?

@wochinge wochinge added type:enhancement ✨ Additions of new features or changes to existing ones, should be doable in a single PR area:rasa-oss 🎡 Anything related to the open source Rasa framework priority:high labels Aug 21, 2020
@wochinge wochinge added this to the 2.0a3 Rasa Open Source milestone Aug 21, 2020
@akelad
Copy link
Contributor

akelad commented Aug 21, 2020

LoopExecutionAfterUnhappyPath pls no 😂 will take a deeper look for better name suggestions

@wochinge
Copy link
Contributor Author

UnhappyLoop? 😄

@wochinge wochinge mentioned this issue Aug 21, 2020
5 tasks
@wochinge
Copy link
Contributor Author

wochinge commented Aug 21, 2020

I created a PR with the whole renaming logic. All which is left todo for Rasa Open Source (this doesn't handle SDK / X yet) is filling in the right name then: #6468

@wochinge
Copy link
Contributor Author

@alwx Did you continue working on this? @akelad Do you have some new name suggestions? 👀

@wochinge wochinge self-assigned this Aug 31, 2020
@Ghostvv
Copy link
Contributor

Ghostvv commented Aug 31, 2020

if you look at loops they don't have validation idea inside them. so technically, Akela's suggestion LoopExecutionAfterUnhappyPath is correct 😅😇

@wochinge
Copy link
Contributor Author

It was mine :-D Akela was unhappy. How about shortening it to LoopUnhappyPath? We might be able to drop this event anyway once we figured out the end-to-end problems for policy predictions.

@Ghostvv
Copy link
Contributor

Ghostvv commented Aug 31, 2020

sorry, didn't see it

@Ghostvv
Copy link
Contributor

Ghostvv commented Aug 31, 2020

How about we keep it FormValidation for now, until we figure out tracker problems?

@Ghostvv
Copy link
Contributor

Ghostvv commented Aug 31, 2020

LoopUnhappyPath hm... smth about it is not intuitive, but cannot formulate what

@akelad
Copy link
Contributor

akelad commented Sep 1, 2020

yeah LoopUnhappyPath doesn't sound right. What's the reason for not calling it LoopValidation again? From what I understand the event gets added to the tracker as FormValidation(False) when the user "breaks out" of a form/loop right? LoopUnhappyPath(False) seems like that would be wrong 😅

@Ghostvv
Copy link
Contributor

Ghostvv commented Sep 1, 2020

FormValidation(False) is added when a form is predicted after a user input that is the answer to a previous question if it were in stories/rules:

- utter_ask_continue
* affirm
- FormValidation(False)
- q_form

but not here (inform is part of the form):

- utter_ask_continue
* inform
- q_form

@Ghostvv
Copy link
Contributor

Ghostvv commented Sep 1, 2020

the problem that currently the implementation of the Loop doesn't have the notion of validation

@akelad
Copy link
Contributor

akelad commented Sep 1, 2020

yeah that part I know.

the problem that currently the implementation of the Loop doesn't have the notion of validation

as in, Loop doesn't, but Forms do right?

@Ghostvv
Copy link
Contributor

Ghostvv commented Sep 1, 2020

yes

@akelad
Copy link
Contributor

akelad commented Sep 1, 2020

so why is the event needed for the Loop at all?

@Ghostvv
Copy link
Contributor

Ghostvv commented Sep 1, 2020

because there is no form in tracker, rule policy, etc anymore. and this event will be added for loops as well, which a custom loop could use, however it wants

@akelad
Copy link
Contributor

akelad commented Sep 1, 2020

yeah but it does nothing for the Loop right now right? Kind of makes it hard to name it anything at all 😂

@wochinge
Copy link
Contributor Author

wochinge commented Sep 1, 2020

Love the discussion 🎉

How about we keep it FormValidation for now, until we figure out tracker problems?

I suggest to do that. There are more important things to do the for the RC at the moment and the event name is not user facing which means we can still do this down the road.

@akelad
Copy link
Contributor

akelad commented Sep 1, 2020

haha yeah for sure, but doesn't hurt to keep the discussion going if someone has a genius idea

@wochinge
Copy link
Contributor Author

wochinge commented Sep 28, 2020

So, what's the state on this? Happy to jump on a 5 minute call to resolve this. @akelad @Ghostvv

What's the reason for not calling it LoopValidation again? From what I understand the event gets added to the tracker as FormValidation(False) when the user "breaks out" of a form/loop right? LoopUnhappyPath(False) seems like that would be wrong 😅

yes, but it might be used in other cases in the future and that's why a general solution makes more sense imo.

@wochinge wochinge removed their assignment Sep 28, 2020
@wochinge
Copy link
Contributor Author

Ok @Ghostvv @akelad .
Sorry to push but this blocks Rasa Open Source and Rasa X:

Candidates:

  • LoopUnhappyPath
  • LoopValidation

Candidates I could also imagine:

  • LoopBreak
  • LoopExemption

I'll move forward implementing it using the name LoopUnhappyPath tomorrow in case we can't agree on another name by then.

@akelad
Copy link
Contributor

akelad commented Sep 28, 2020

What about LoopInterruption? but then I guess the event would be LoopInterruption(True) rather than LoopInterruption(False)? Otherwise it sounds wrong. If we don't wanna switch around the True/False parameters though I would prob just stick with Validation?

@wochinge
Copy link
Contributor Author

I like LoopInterruption. Gonna use that then. I can swap the true/false stuff when deprecating the FormValidation event. Let's hear if Vova has strong feelings against this tomorrow and then we use LoopInterruption

@Ghostvv
Copy link
Contributor

Ghostvv commented Sep 29, 2020

this event is added before returning to the loop, and is used to notify the loop that it is called after interruption. maybe LoopInterrupted(true) then?

@akelad
Copy link
Contributor

akelad commented Sep 29, 2020

Yes that’s even better!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:rasa-oss 🎡 Anything related to the open source Rasa framework type:enhancement ✨ Additions of new features or changes to existing ones, should be doable in a single PR
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants