-
Notifications
You must be signed in to change notification settings - Fork 17
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
Command line parameters for search paths #89
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.
I'd like to suggest some small changes before merging this pull request. I think you should swap the -I
and -L
options so that -I
configures local_search_dirs
and -L
configures lib_search_dirs
. While the search paths don't accurately correspond to the traditional flags from C-related infrastructure, for now I believe this way will be more intuitive, unless you can convince me otherwise.
By the way, using -I
or -L
multiple times for every directory seems fine, and space-separated arguments to these options would clash with how all anonymous arguments after the filename are treated as arguments for the interpreted program anyway. So, I think this part is good as is.
src/dbl.ml
Outdated
" Make internal errors more verbose (for debugging only)"; | ||
|
||
"-I", | ||
Arg.String (fun p -> Config.lib_search_dirs := !Config.lib_search_dirs @ [p]), |
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.
Expressions like xs @ [x]
performed in a loop tend to look a bit fishy (though, to be fair, these lists are unlikely to be long). Besides, I'm not sure that we actually want to add directories from command-line options at the very end; perhaps they should take precedence over some other potential sources (e.g., environment variables), even if presently it makes no difference. I think you can solve both of these minor issues by accumulating these directories in reverse order into a temporary list, and List.rev_append
ing them with the list in Config
after parsing all arguments.
Also, this line is too long.
src/dbl.ml
Outdated
|
||
"-I", | ||
Arg.String (fun p -> Config.lib_search_dirs := !Config.lib_search_dirs @ [p]), | ||
" Standard library paths"; |
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 option takes just one path, which I don't think is clear from the description. I'd also prefer it to be phrased in terms of an action that the option will perform, like "add a directory ...", to be consistent with the other help messages.
src/dbl.ml
Outdated
Arg.String (fun p -> Config.local_search_dirs := !Config.local_search_dirs @ [p]), | ||
" Local search paths"; |
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.
Same feedback as above.
f2efc2a
to
469e3d3
Compare
I agree that swapping the meaning of the flags is a good idea. I added changes you suggested. |
4174993
to
d1687ec
Compare
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 good, thanks!
This commit should resolve #85.
I am not sure if it is possible have functionality of specifying multiple arguments to a single option, like
dbl -I path1 path2 source.dbl
with Arg API easily.