-
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
eskimo--assignment1 (Command Line Parser) #51
base: master
Are you sure you want to change the base?
Conversation
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.
Looks like you've gone a bit overboard on the comments, you want to use them only when actually needed. There is a balance. Ie - if I can read code, the comment explaining what it does is pointless. But if you have a large method (like 50 lines of code) then having a 2-3 line comment for me to read that describes what you're trying to do is useful.
@@ -0,0 +1,45 @@ | |||
var command_line=require("./command_line"); |
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.
Lets use a framework like Jest to test your code properlu.
this.argument_JSON={}; | ||
|
||
//To check two Exclusive commands doesn't get together | ||
this.isSetExclusive= new Array(100).fill("0"); |
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.
I always thought that the linter wanted spaces around operations like '='.
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.
I used the linter of visual studio code.
type:'number', | ||
handler: function(){ | ||
console.log("Key"); | ||
} |
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.
Looks like the helper is called when the this command is provided?
That can be useful, but it isn't strictly necessary.
Ie - when I'm using a CLI argument parser, each command does not map to a specific method.
It might change something small, like a condition in an if-statement in some other part of the code.
But I guess it's fine now that you've added it.
{ | ||
command:"help", | ||
describe:"HELP", | ||
handler:()=>{ |
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.
These instructions are useful if you're a developer using the library, but not if you're a user of the larger program.
And so showing them in CLI output is not really useful I think. It would have been fine to explain all this in a comment next to the method that you're supposed to call, or in a readme file.
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.
I checked that yargs have such type of property of providing command details when --help is typed, that's why I included it.
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.
I'm not opposed to have a help.
There are 3 roles here:
- the developer of the library (you)
- the developer of the application (using this library)
- the user of the application
This output will be seen by "the user of the application", but is actually relevant only to "the developer of the application". If you want to show something for "the user of the application", you could create a help message based on the exact commands that were specified. This is where the description field comes in handy :)
* Add commands to commandList | ||
* @param {JSON_object} add_command | ||
*/ | ||
command(add_command) |
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 could also provided default values like this:
add_option({command, type = 'string', required = false, ...})
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Destructuring_assignment
Btw, note that I renamed the method. Because a function named "command" does not tell me what it does without looking inside (ie - is it get_command? or does_command_exist?).
command_line.command({ | ||
command:"local", | ||
describe:"Set local", | ||
ExclusiveIndex: 1, |
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.
This is not great API. Can you do something like:
group = command_line.add_mutually_exclusive_group()
group.add('local', ...)
group.add('remote', ...)
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.
I think of creating a commad line parser like yargs... so I used its format of input.
if(!val.includes("=")) | ||
{ | ||
//Error if argument doesn't have a value | ||
console.log("The '--"+command_available.command+"' argument is required, but missing from input."); |
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.
Instead of logging, you should throw errors here too, right?
} | ||
|
||
//Adds help to commandList | ||
this.commandList.push(help_obj); |
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.
This shouldn't be a class property.
Ie - it is relevant only for a single call to your execute
method, so you can create it there, return it, and then forget it.
//Checks the command type | ||
var command_type=command_available.type; | ||
|
||
if(command_type=='string') |
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.
How about a map?
const validators = {
string: value => value.match("^[A-z]+$"),
number: value => value.match("^[0-9]+$"),
...
}
You'll save a lot of lines.
/** | ||
* Prints the JSON | ||
*/ | ||
display() |
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.
this.argument_JSON
shouldn't be a class property.
Ie - it is relevant only for a single call to your execute method, so you can create it there, return it, and then forget it. If the caller wants, they can remember it, but that's not the job of this module.
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.
added small argument option (-k), renamed command to add_command, reduced extra comments, mapped the commandName with command, removed arg_JSON & commandList from class property,
throw error instead of logging it.
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.
Still waiting to address a few comments from previous review.
{ | ||
command:"help", | ||
describe:"HELP", | ||
handler:()=>{ |
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.
I'm not opposed to have a help.
There are 3 roles here:
- the developer of the library (you)
- the developer of the application (using this library)
- the user of the application
This output will be seen by "the user of the application", but is actually relevant only to "the developer of the application". If you want to show something for "the user of the application", you could create a help message based on the exact commands that were specified. This is where the description field comes in handy :)
var commandsmallName={}; | ||
|
||
//Stores the argument in JSON format | ||
var argument_JSON={}; |
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.
There shouldn't be any global variables like this. This violates OOPs :)
If your parser class needs anything, it can create class properties.
f9e6d92
to
4bfac84
Compare
No description provided.