Add Solution for Assignment 1 #57
Conversation
| return r'^[0-9]+$' | ||
| return r'^\w+$' | ||
|
|
||
| OPTIONS = Parser() |
There was a problem hiding this comment.
If you want to test stuff like this, you typically should do this from a separate file?
That way, you have a clear separate between library vs application.
| self.arguments.append((str(command), str(value))) | ||
| if len(arg_pair) == 1: | ||
| self.validate(command, 'True') | ||
| self.arguments.append((str(command), 'True')) |
There was a problem hiding this comment.
- Why do you need to save this in a class property like
self.arguments? Why not something simpler like local variable? - How about use adding the value to a dict directly, instead of using tuples as an intermediate step?
| """Ensures that command and value are valid""" | ||
| currently_valid = False | ||
| for option in self.options: | ||
| if bool(re.match(option.option_name, str(command), re.IGNORECASE)): |
There was a problem hiding this comment.
Looks like you're just doing a string contains check.
if option.option_name in command
| if required: | ||
| self.required_options.append(option_being_added) | ||
|
|
||
| def add_mutually_exclusive_options(self, option1, option2): |
There was a problem hiding this comment.
There can be more than 2 mutually exclusive things.
| option_being_added = Option(option_name, option_description, option_type) | ||
| self.options.append(option_being_added) | ||
| if required: | ||
| self.required_options.append(option_being_added) |
There was a problem hiding this comment.
Instead of a separate list, why not an extra flag in the options object?
kaustubh-karkare
force-pushed
the
master
branch
from
f9e6d92 to
4bfac84
Compare
| def validate_type_and_existence(self, command, value): | ||
| """Ensures that command and value are valid""" | ||
| currently_valid = False | ||
| for option in self.options: |
There was a problem hiding this comment.
Using a dict instead of a list will reduce complexity here.
| if option.is_required is True: | ||
| currently_valid = False | ||
| for commands in arguments: | ||
| if commands == option.option_name: |
There was a problem hiding this comment.
for option in self.options:
if option.option_name in arguments:
| option_being_added = Option(option_name, option_description, option_type, required) | ||
| self.options.append(option_being_added) | ||
|
|
||
| def add_mutually_exclusive_options(self, exclusive_options_list): |
There was a problem hiding this comment.
Lets add a required flag here also? So that we can conditionally require that at least one of the mutually exclusive options are provided.
| isRequired - bool value to check if option is required or not | ||
| """ | ||
| def __init__(self, option_name, option_description, option_type, is_required): | ||
| self.option_name = option_name |
There was a problem hiding this comment.
Given that we access this as option.option_name, we could have just called it name (ie - option.name).
|
|
||
| def add_mutually_exclusive_options(self, exclusive_options_list, is_one_required): | ||
| """adds conflicting commands to a list as a tuple""" | ||
| self.mutually_exclusive_options.append((exclusive_options_list, is_one_required)) |
There was a problem hiding this comment.
Btw, the name mutually_exclusive_options is not enough to indicate what it contains now.
Just something to be aware of :)
No description provided.