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

Solution to assignment-1 added #54

Open
wants to merge 14 commits into
base: master
Choose a base branch
from
1 change: 1 addition & 0 deletions solutions/ayush-shah/argument-parser/.gitignore
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
*.pyc
24 changes: 24 additions & 0 deletions solutions/ayush-shah/argument-parser/README.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
# Examples
Copy link
Owner

Choose a reason for hiding this comment

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

You're using this readme file to prove that the command works.
Instead, just use a proper testing framework like Jest.

Copy link
Owner

Choose a reason for hiding this comment

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

You can get rid of this file now, since (a) the test proves that it works, and (b) these examples do not work anymore.

```
$ python3 parser.py --key=1234 --name=ayush
{ "--key": "1234, "--name": "ayush" }

$ python3 parser.py --key=ayush
Error: The value for argument '--key' must be integer

$ python3 parser.py --name=ayush
Error: The argument '--key' is required, but missing from input

$ python3 parser.py
Error: The argument '--key' is required, but missing from input

$ python3 parser.py --local --remote
Error: The '--local' and '--remote' arguments cannot be used together
Error: The argument '--key' is required, but missing from input

$ python3 parser.py --key=1234 --roll=10014
Error: invalid argument '--roll'

$ python3 parser.py --key
Error: The value for argument '--key' is missing
```
94 changes: 94 additions & 0 deletions solutions/ayush-shah/argument-parser/parser.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,94 @@
import sys
import json

class Parser:
"""
Class for adding the command line arguments
storing errors, storing results and displaying them
"""
Integer, String, Others, Required = set(), set(), set(), set()
Copy link
Owner

Choose a reason for hiding this comment

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

  • These are static properties on the class which prevents multiple parser objects from being created and used simultaneously. We shouldn't need these, we can create normal properties.
  • We shouldn't create a separate set for each type. Instead, you should store type information as part of an argument object, and a list of argument objects can be stored on the parser object.

Json = dict()

def add_argument(self, argument, required, types):
if types == 'integer':
self.Integer.add(argument)
if required == 'yes':
self.Required.add(argument)
elif types == 'string':
self.String.add(argument)
if required == 'yes':
self.Required.add(argument)
else:
self.Others.add(argument)
if required == 'yes':
self.Required.add(argument)

def store_result(self, key, value):
self.Json[key] = value

def check_required(self, keys):
for check in self.Required:
flag = False
for key in keys:
if key == check:
flag = True
if not flag:
return False, check
return True, "ok"

def check_local_and_remote(self, argument):
length_of_arguments = len(argument)
check = False
for arguments in range(1, length_of_arguments):
args = argument[arguments]
key = args.partition('=')[0]
if key == '--local' and not check:
check = True
elif key == '--remote' and not check:
check = True
elif check and key in ('--local', '--remote'):
return True
return False
Comment on lines +50 to +62
Copy link
Owner

Choose a reason for hiding this comment

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

This is not allowed.
You cannot assume that --local and --remote are the only mutually exclusive options. Instead, you need to make that configurable, just like add_option


def display_result(self):
to_json = json.dumps(self.Json)
return to_json

def main(self, argument):
self.add_argument('--key', 'yes', 'integer')
self.add_argument('--name', 'no', 'string')
self.add_argument('--local', 'no', 'others')
self.add_argument('--remote', 'no', 'others')
Copy link
Owner

Choose a reason for hiding this comment

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

You shouldn't be adding arguments in this file at all.
Each test method can add its own arguments, but the code in this file needs to be very generalized and reusable.

length_of_arguments = len(argument)
if length_of_arguments == 1:
return "Error: no arguments given in input"
if self.check_local_and_remote(argument):
return "Error: The '--local' and '--remote' arguments cannot be used together"
Copy link
Owner

Choose a reason for hiding this comment

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

Things like --local and --remote should never appear in the code of this parser module, since we're supposed to make it configurable. You want to make this configurable in your API.

keys = list()
for arguments in range(1, length_of_arguments):
args = argument[arguments]
key = args.partition('=')[0]
value = args.partition('=')[2]
Comment on lines +77 to +78
Copy link
Owner

Choose a reason for hiding this comment

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

More concise:

key, _, value = args.partition('=')

keys.append(key)
if key not in ("--remote", "--local"):
self.store_result(key, value)
Copy link
Owner

Choose a reason for hiding this comment

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

You shouldn't save the result before doing the validation below.

if (key not in self.Integer) and (key not in self.String) and(key not in self.Others):
return "Error: invalid argument '" + key + "'"
if '=' not in args:
return "Error: The value for argument '" + key + "' is missing"
if key in self.Integer:
if not value.isdigit():
return "Error: The value for argument '" + key + "' must be integer"
if key in self.String:
if not value.isalpha():
return "Error: The value for argument '" + key + "' must be string"

response, key = self.check_required(keys)
if not response:
return "Error : argument '" + key + "' is required but missing"
return self.display_result()
Copy link
Owner

Choose a reason for hiding this comment

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

This method should either return a dict, or raise an exception.
Returning strings is not good API, since the calling method needs to look at string contents to figure out whether the operation worked or not.


if __name__ == '__main__':
Copy link
Owner

Choose a reason for hiding this comment

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

Don't need a main section at all, since this is supposed to be a reusable library.

Parse = Parser()
Parse.main(sys.argv)

48 changes: 48 additions & 0 deletions solutions/ayush-shah/argument-parser/test.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,48 @@
import unittest
import parser as parse

class UnitTesting(unittest.TestCase):
parser = parse.Parser()
def test_basic_usage(self):
command_line_parameters = ['./test', '--key=1234', '--name=ayush']
result = self.parser.main(command_line_parameters)
self.assertEqual(result, '{"--key": "1234", "--name": "ayush"}')

def test_invalid_key(self):
command_line_parameters = ['./test', '--key=ayush', '--name=ayush']
result = self.parser.main(command_line_parameters)
self.assertEqual(result, "Error: The value for argument '--key' must be integer")

def test_required_argument(self):
command_line_parameters = ['./test', '--name=ayush']
result = self.parser.main(command_line_parameters)
self.assertEqual(result, "Error : argument '--key' is required but missing")

def test_invalid_name(self):
command_line_parameters = ['./test', '--key=1234', '--name=1234']
result = self.parser.main(command_line_parameters)
self.assertEqual(result, "Error: The value for argument '--name' must be string")

def test_no_arguments(self):
command_line_parameters = ['./test']
result = self.parser.main(command_line_parameters)
self.assertEqual(result, "Error: no arguments given in input")

def test_local_and_remote(self):
command_line_parameters = ['./test', '--local', '--remote']
result = self.parser.main(command_line_parameters)
self.assertEqual(result, "Error: The '--local' and '--remote' arguments cannot be used together")

def test_unrecognized_command(self):
command_line_parameters = ['./test', '--roll']
result = self.parser.main(command_line_parameters)
self.assertEqual(result, "Error: invalid argument '--roll'")

def test_valueless_argument(self):
command_line_parameters = ['./test', '--key']
result = self.parser.main(command_line_parameters)
self.assertEqual(result, "Error: The value for argument '--key' is missing")


if __name__ == '__main__':
unittest.main()