-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
Replace ValueError
with RasaException
in replace_environment_variables
function
#8382
Conversation
rasa/shared/utils/io.py
Outdated
@@ -314,7 +315,7 @@ def env_var_constructor(loader, node): | |||
w for w in expanded_vars.split() if w.startswith("$") and w in value | |||
] | |||
if not_expanded: | |||
raise ValueError( | |||
raise RasaException( | |||
"Error when trying to expand the environment variables" | |||
" in '{}'. Please make sure to also set these environment" | |||
" variables: '{}'.".format(value, not_expanded) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
More of a style comment, shouldn't this be updated to a f-string according to Notion code convention ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good catch!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks all good with me 👍 although not sure why the docstring for read_yaml
mentions a 3rd argument that doesn't seem present in the function signature?
@ancalita I guess it's a leftover. I will remove it :) |
Proposed changes:
ValueError
withRasaException
inreplace_environment_variables
function because it's indeed a usage errorread_yaml
function failing when trying to expand an environment variable which contains a$
#8140Status (please check what you already did):
black
(please check Readme for instructions)