-
Notifications
You must be signed in to change notification settings - Fork 21
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
base: master
Are you sure you want to change the base?
Changes from all commits
f928969
ed3124c
a11a9f5
74ea32f
9516e8b
d18f584
3d56391
b509bc0
d6eb0a9
283091f
d8568a0
1097ea5
b1774fb
0350c26
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
*.pyc |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,23 @@ | ||
# Examples | ||
``` | ||
$ 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 | ||
``` |
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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 = {} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You want the code you write to be easily readable. |
||
self.arguments.append(add) | ||
|
||
def store_result(self, key, value): | ||
self.Json[key] = value | ||
|
||
def check_required(self, keys): | ||
for check in self.arguments: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is not allowed. |
||
|
||
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
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. More concise:
|
||
keys.append(key) | ||
if key not in ("--remote", "--local"): | ||
self.store_result(key, value) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This method should either return a dict, or raise an exception. |
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() |
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.
You're using this readme file to prove that the command works.
Instead, just use a proper testing framework like Jest.
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.
You can get rid of this file now, since (a) the test proves that it works, and (b) these examples do not work anymore.