-
Notifications
You must be signed in to change notification settings - Fork 1
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
Syntax errors branch #17
base: master
Are you sure you want to change the base?
Conversation
env_spec.py
Outdated
@@ -124,50 +126,57 @@ def parse(env_spec_text): | |||
comment_regex = r"^(.+)\#(.+)$" | |||
|
|||
lines = env_spec_text.split("\n") | |||
enumerated_list = list(enumerate(lines)) |
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.
I think we do not have to wrap enumerate(...)
in a list
, right?
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.
if I have only enumerated_list = (enumerate(lines)), I will get the enumerated object.
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.
Won't the enumerated object work like a list in a for ... in
loop?
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.
Yes, it will.My mistake.
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.
Not a problem at all 😄!
env_spec.py
Outdated
|
||
is_variable_name_valid = re.match( | ||
alphanumeric_that_does_not_start_with_digit, name | ||
) | ||
|
||
if not is_variable_name_valid: | ||
raise EnvSpecSyntaxError("SYNTAX ERROR: Invalid variable name.") | ||
raise EnvSpecSyntaxError( | ||
"SYNTAX ERROR: Invalid variable name.", line_number |
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.
Can you change the error message to this please?
Invalid variable name; it should contain only latin alphanumeric characters, underscores and not start with a digit.
env_spec.py
Outdated
|
||
if choices_match: | ||
choices_str = choices_match.groups()[0] | ||
|
||
choices = create_list(choices_str) | ||
|
||
if not choices: | ||
raise EnvSpecSyntaxError("SYNTAX ERROR: Invalid choices list.") | ||
raise EnvSpecSyntaxError( | ||
"SYNTAX ERROR: Invalid choices list.", line_number |
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.
Can you change the error message to this please?
Invalid restricted choices syntax.
env_spec.py
Outdated
@@ -177,32 +186,36 @@ def parse(env_spec_text): | |||
|
|||
if choices_match: | |||
if default_value not in choices or default_value == "": | |||
raise EnvSpecSyntaxError("SYNTAX ERROR: Invalid default value.") | |||
raise EnvSpecSyntaxError( | |||
"SYNTAX ERROR: Invalid default value.", line_number |
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.
Can you change the error message to this please?
Invalid default value; it is not included in the provided restricted choices.
env_spec.py
Outdated
|
||
env_spec_type = env_spec_type.strip() | ||
|
||
if env_spec_type not in valid_types_list: | ||
raise EnvSpecSyntaxError("SYNTAX ERROR: Invalid type.") | ||
raise EnvSpecSyntaxError("SYNTAX ERROR: Invalid type.", line_number) |
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.
Can you change the error message to this please?
Invalid variable type; it should be one of "bla", "blah", "blah".
P.S.: @krantou do not hardcode the valid types in the error message above, but construct the substring by joining the elements of valid_types_list
.
is_variable_name_valid = re.match( | ||
alphanumeric_that_does_not_start_with_digit, name | ||
) | ||
|
||
if not is_variable_name_valid: | ||
raise EnvSpecSyntaxError("SYNTAX ERROR: Invalid variable name.") | ||
raise EnvSpecSyntaxError( |
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.
Can you change the error message to this please?
Invalid variable name; it should contain only latin alphanumeric characters, underscores and not start with a digit.
Also, why are we validating the variable name in two places?
No description provided.