Solution to assignment-1 added #54
Conversation
| @@ -0,0 +1,27 @@ | |||
| # Examples | |||
There was a problem hiding this comment.
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.
You can get rid of this file now, since (a) the test proves that it works, and (b) these examples do not work anymore.
| @@ -0,0 +1,99 @@ | |||
| import sys | |||
| Integer, String, Others, Required = set(), set(), set(), set() | |||
| errors, Json = dict(), dict() | |||
There was a problem hiding this comment.
You shouldn't put these outside the class, they can all be class properties / local variables.
| return 0 | ||
| check = False | ||
| keys = list() | ||
| for arguments in range(1, length_of_arguments): |
There was a problem hiding this comment.
This stuff should also be inside the class.
| parse.display_result() | ||
|
|
||
| if __name__ == '__main__': | ||
| main(sys.argv) |
There was a problem hiding this comment.
This is intended to be a library, so I'd recommend not doing any work in this file.
Just implement the class, and then from the test file, you can use it.
| if not flag: | ||
| return False, check | ||
| return True, "ok" | ||
| def display_error(self): |
There was a problem hiding this comment.
I like that you're displaying errors, but in general, displaying isn't enough.
This is just a library that will be called by a larger application, so you should raise exceptions instead.
The caller can then choose to display the errors, or do something else.
| print("Error: The value for argument '"+key+"' must be integer") | ||
| if error == "not-string": | ||
| print("Error: The value for argument '"+key+"' must be string") | ||
| def display_result(self): |
There was a problem hiding this comment.
This is not true JSON.
Check out the json module.
kaustubh-karkare
force-pushed
the
master
branch
from
f9e6d92
to
4bfac84
Compare
| self.add_argument('--key', 'yes', 'integer') | ||
| self.add_argument('--name', 'no', 'string') | ||
| self.add_argument('--local', 'no', 'others') | ||
| self.add_argument('--remote', 'no', 'others') |
There was a problem hiding this comment.
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.
| response, key = self.check_required(keys) | ||
| if not response: | ||
| return "Error : argument '" + key + "' is required but missing" | ||
| return self.display_result() | ||
|
|
||
| if __name__ == '__main__': |
There was a problem hiding this comment.
Don't need a main section at all, since this is supposed to be a reusable library.
| 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" |
There was a problem hiding this comment.
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.
| response, key = self.check_required(keys) | ||
| if not response: | ||
| return "Error : argument '" + key + "' is required but missing" | ||
| return self.display_result() |
There was a problem hiding this comment.
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.
| Class for adding the command line arguments | ||
| storing errors, storing results and displaying them | ||
| """ | ||
| Integer, String, Others, Required = set(), set(), set(), set() |
There was a problem hiding this comment.
- 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.
| def add_argument(self, argument, required, types): | ||
| add = Options(argument, types, required) |
There was a problem hiding this comment.
You want the code you write to be easily readable.
This change in ordering of arguments is random and therefore confusing.
|
|
||
| def __init__(self, argument, types, required): | ||
| self.argument = argument | ||
| self.types = types |
There was a problem hiding this comment.
The variable name "types" suggests a list of things, not a single string, which is what you're providing.
| 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 |
There was a problem hiding this comment.
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 __init__(self): | ||
| self.arguments = [] | ||
| self.Json = {} |
There was a problem hiding this comment.
Don't save the result on the class, just return it.
| key = args.partition('=')[0] | ||
| value = args.partition('=')[2] |
There was a problem hiding this comment.
More concise:
key, _, value = args.partition('=')
| value = args.partition('=')[2] | ||
| keys.append(key) | ||
| if key not in ("--remote", "--local"): | ||
| self.store_result(key, value) |
There was a problem hiding this comment.
You shouldn't save the result before doing the validation below.
| 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.
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.
| self.Json[key] = value | ||
|
|
||
| def check_required(self, keys): | ||
| for check in self.arguments: |
There was a problem hiding this comment.
"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.
| for key in keys: | ||
| if key == check.argument and check.required: | ||
| flag = True |
There was a problem hiding this comment.
This is an unnecessary O(n) operation. You can optimize it to O(1).
No description provided.