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
23 changes: 23 additions & 0 deletions solutions/ayush-shah/argument-parser/README.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
# 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

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

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

class ParseError(Exception):
"""
Class for raising exceptions
"""

def __init__(self, message):
super(ParseError, self).__init__(message)

class Options(object):
"""
Class for storing argument information
"""

def __init__(self, argument, types, required):
self.argument = argument
self.types = types
Copy link
Owner

Choose a reason for hiding this comment

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

The variable name "types" suggests a list of things, not a single string, which is what you're providing.

self.required = required

class Parser:
"""
Class for adding the command line arguments
storing errors, storing results and displaying them
"""

def __init__(self):
self.arguments = []
self.Json = {}
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 save the result on the class, just return it.


def add_argument(self, argument, required, types):
add = Options(argument, types, required)
Comment on lines +31 to +32
Copy link
Owner

Choose a reason for hiding this comment

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

You want the code you write to be easily readable.
This change in ordering of arguments is random and therefore confusing.

self.arguments.append(add)

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

def check_required(self, keys):
for check in self.arguments:
Copy link
Owner

Choose a reason for hiding this comment

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

"check" is not a useful variable name, since I have no idea what it contains without looking at the usage. The proper name here is "option" or something.

flag = False
if not check.required:
continue
for key in keys:
if key == check.argument and check.required:
flag = True
Comment on lines +43 to +45
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 an unnecessary O(n) operation. You can optimize it to O(1).

if not flag:
return False, check.argument
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):
length_of_arguments = len(argument)
if length_of_arguments == 1:
raise ParseError("Error: no arguments given in input")
if self.check_local_and_remote(argument):
raise ParseError("Error: The '--local' and '--remote' arguments cannot be used together")
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.

flag = False
for obj in self.arguments:
if obj.argument == key:
flag = True
if not flag:
raise ParseError("Error: invalid argument '" + key + "'")
if '=' not in args:
raise ParseError("Error: The value for argument '" + key + "' is missing")
flag = False
for obj in self.arguments:
if obj.types == "integer" and obj.argument == key:
Copy link
Owner

Choose a reason for hiding this comment

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

Using strings like "integer" in your code is bad practice, because it is easy to make typing mistakes. Use constants defined at the top of your file, or enums, etc.

flag = True
if flag:
if not value.isdigit():
raise ParseError("Error: The value for argument '" + key + "' must be integer")
if not flag:
if not value.isalpha():
raise ParseError("Error: The value for argument '" + key + "' must be string")

response, key = self.check_required(keys)
if not response:
raise ParseError("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.

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

class UnitTesting(unittest.TestCase):
parser = parse.Parser()

def test_basic_usage(self):
self.parser.add_argument('--key', True, 'integer')
self.parser.add_argument('--name', False, 'string')
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):
self.parser.add_argument('--key', True, 'integer')
self.parser.add_argument('--name', False, 'string')
with self.assertRaises(parse.ParseError) as error:
self.parser.main(['./test', '--key=ayush', '--name=ayush'])
self.assertEqual("Error: The value for argument '--key' must be integer", str(error.exception))

def test_required_argument(self):
self.parser.add_argument('--key', True, 'integer')
self.parser.add_argument('--name', False, 'string')
with self.assertRaises(parse.ParseError) as error:
self.parser.main(['./test', '--name=ayush'])
self.assertEqual("Error : argument '--key' is required but missing", str(error.exception))

def test_invalid_name(self):
self.parser.add_argument('--key', True, 'integer')
self.parser.add_argument('--name', False, 'string')
with self.assertRaises(parse.ParseError) as error:
self.parser.main(['./test', '--key=1234', '--name=1234'])
self.assertEqual("Error: The value for argument '--name' must be string", str(error.exception))

def test_no_arguments(self):
self.parser.add_argument('--key', True, 'integer')
self.parser.add_argument('--name', False, 'string')
with self.assertRaises(parse.ParseError) as error:
self.parser.main(['./test'])
self.assertEqual("Error: no arguments given in input", str(error.exception))

def test_local_and_remote(self):
self.parser.add_argument('--local', False, 'others')
self.parser.add_argument('--remote', False, 'others')
with self.assertRaises(parse.ParseError) as error:
self.parser.main(['./test', '--local', '--remote'])
self.assertEqual("Error: The '--local' and '--remote' arguments cannot be used together", str(error.exception))

def test_unrecognized_command(self):
self.parser.add_argument('--key', True, 'integer')
self.parser.add_argument('--name', False, 'string')
self.parser.add_argument('--local', False, 'others')
self.parser.add_argument('--remote', False, 'others')
with self.assertRaises(parse.ParseError) as error:
self.parser.main(['./test', '--roll'])
self.assertEqual("Error: invalid argument '--roll'", str(error.exception))

def test_valueless_argument(self):
self.parser.add_argument('--key', True, 'integer')
with self.assertRaises(parse.ParseError) as error:
self.parser.main(['./test', '--key'])
self.assertEqual("Error: The value for argument '--key' is missing", str(error.exception))

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