-
Notifications
You must be signed in to change notification settings - Fork 234
Refactor all wrappers to pass an argument list to Session.call_module #3132
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
Conversation
82d7035 to
b52469f
Compare
14e6ccc to
9fb52d6
Compare
|
Seriously? After all these years and countless workarounds, and there was a simpler way?!! |
😄 Should have read the C source codes earlier. |
b799d5c to
d44e494
Compare
234cdf1 to
816cafd
Compare
This comment was marked as outdated.
This comment was marked as outdated.
eafc3e0 to
df3ffdc
Compare
|
After PR #3139 and #3149, now it's time to refactor module wrappers to pass a list of arguments when calling modules. More than 60 files are changed in this PR but in most files, the changes are simple (just replacing
|
|
/format |
|
Ping @GenericMappingTools/pygmt-maintainers for final review call. Will merge in 48 hours if no further comments. |
weiji14
left a comment
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!
|
|
||
| @fmt_docstring | ||
| @use_alias(G="download", V="verbose") | ||
| @kwargs_to_strings(fname="sequence_space") |
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.
Don't need to do it in this PR, but should we remove sequence_space from decorators.py at some point?
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.
Absolutely yes.
No longer needed after build_arg_list was used in #3132.
TL;DR
When calling a GMT module, we pass a single text string of CLI arguments (e.g.,
"-R0/10/0/10 -JX10c -BWSen+tMy Title") to the C API. It has caused many troubles if an argument contains whitespaces (e.g., #247, #655, #2298 and more) or single/double quotation marks (e.g., #3104 and more), and we have to introduce some hacking solutions (e.g., #1487).Instead of passing a single text string, we can pass a list of argument string instead (e.g.,
["-R0/10/0/10", "-JX10c", "-BWSen+tMy Title"]. In this way, we no longer need any hack solutions and everything just works as expected!Technical backgrounds
Currently, the
Session.call_module()method have two parameters:moduleis name of a GMT module, andargsis the text string of CLI arguments. For example, we usually call the method like this way:in which
build_arg_stringconverts a keyword dictionary into a single text string for CLI arguments (e.g.,"-R0/10/0/10 -JX10c -BWSen+tMy Title").Actually, the C API function
GMT_Call_Moduleallows passing CLI arguments in three different modes:"-R0/10/0/10 -JX10c -BWSen+tMy Title"["-R0/10/0/10", "-JX10c", "-BWSen+tMy Title"]GMT_OPTIONstructureCurrently, we use mode 1. As mentioned above, it has caused many trouble if an argument has whitespace and single/double quotation marks.
GMT_Call_Module'smodeparameter is actually passed to then_args_inparameter in another C API functionGMT_Create_Options. Here is the description of theGMT_Create_Optionsfunction:So, if mode 1 is used, the
GMT_Create_Optionsfunction first splits the single text string into an array of strings (i.e., mode 2), and then converts the array of strings to a list of GMT_OPTION structure (i.e., mode 3). As can be seen in theGMT_Create_Optionssource codes, for mode 1, GMT tries very hard to deal with whitespace and quotation marks and we're repeating the same work in thebuild_arg_stringfunction. Mode 1 is definitely the "hardest" mode for us and mode 3 means we have to wrap the GMT_OPTION structure, so why not just use mode 2!Changes in this PR
Session.call_moduleaccept a list of strings (i.e., mode 2). Passing a single string is still supported for backward-compatibility Session.call_module: Support passing a list of argument strings #3139build_arg_listfunction which does the same thing asbuild_arg_stringbut returns a list. Thebuild_arg_listdoesn't need any hacking solutions as inbuild_arg_string. Add function build_arg_list for building arguments list from keyword dictionaries #3149build_arg_stringwithbuild_arg_listin all module wrappersSession.call_modulemethoddata=["file1.txt", "file2.txt"],data_kind(data)returnsmatrix, which cause failures. I guess we need to refactor thedata_kindfunction.To Be Done/Decided
Raise a deprecation warning when passing a single string toLikely no, because sometimes passing a single string is convenient but we should avoid using it in the PyGMT source codes.Session.call_moduleand fully remove the support in v0.20.0 or even v1.0?build_arg_stringfunction for some time because we know some users write their own wrappers and use thebuild_arg_stringfunction. A deprecation warning makes sense.Benifits
Figure.plot(data=["file1.txt", "file2.txt"])Supersede PRs #1487, #1116, #2331, #2726.
Also fixes issue #3104 without the solution in #3105, but I'll open some more discussions about quotation marks, so this PR won't close issue #3104.