Skip to content
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.

Sign up for GitHub

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

Disallow passing arguments like -XNone to GMT #639

Merged
merged 4 commits into from
Oct 8, 2020
Merged

Disallow passing arguments like -XNone to GMT #639

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
7a9d24b
Disallow passing argumenst like -XNone to GMT
seisman Oct 4, 2020
95dbdca
Merge branch 'master' into remove-None-args
seisman Oct 5, 2020
b2e9acf
Format codes
seisman Oct 5, 2020
c644df7
Merge branch 'master' into remove-None-args
seisman Oct 8, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 7 additions & 1 deletion pygmt/helpers/utils.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -127,7 +127,11 @@ def build_arg_string(kwargs):
Examples
--------

>>> print(build_arg_string(dict(R="1/2/3/4", J="X4i", P="", E=200)))
>>> print(
... build_arg_string(
... dict(R="1/2/3/4", J="X4i", P="", E=200, X=None, Y=None)
... )
... )
-E200 -JX4i -P -R1/2/3/4
>>> print(
... build_arg_string(
Expand All @@ -147,6 +151,8 @@ def build_arg_string(kwargs):
if is_nonstr_iter(kwargs[key]):
for value in kwargs[key]:
sorted_args.append("-{}{}".format(key, value))
elif kwargs[key] is None: # arguments like -XNone are invalid
continue
Comment on lines +154 to +155
Copy link
Member

@weiji14 weiji14 Oct 5, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So X=None should return the same result as X=False, following what was suggested at #380 (comment).

Question is, should the None detection logic be placed here, or would it be better to put it in remove_bools somewhere here:

pygmt/pygmt/helpers/decorators.py

Lines 436 to 443 in 1bf0a30

new_kwargs = {}
for arg, value in kwargs.items():
if isinstance(value, bool):
if value:
new_kwargs[arg] = ""
else:
new_kwargs[arg] = value
return new_kwargs

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#640 explains why we can't deal with None or even boolean values in the kwargs_to_strings decorator.

Copy link
Member

@weiji14 weiji14 Oct 5, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is very confusing 😕, so for most PyGMT modules, we go through 1) kwargs_to_strings decorator, 2) some PyGMT virtualfile logic or whatever, and then 3) the build_arg_string which passes things to GMT C API.

In (1), the kwargs_to_string has a remove_bools setting that turns True into an empty string "", are you suggesting in #640 that we should not perform that True to "" conversion in (1), but move it to (3) at build_arg_string instead? Similarly in this PR, None has to be handled later at step (3) in build_arg_string, rather than at step (1) in kwargs_to_string/remove_bools.

Copy link
Member Author

@seisman seisman Oct 5, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, as shown in #640, the boolean argument nongmt_args should be True or False. However, when calling the function, the decorator kwargs_to_string converts nongmt_args=True to nongmt_args="". It makes sense if nongmt_args is an alias of GMT options (e.g., -N), so that we can pass -N to GMT. However, when nongmt_args is just a Python variable, the conversion means checking nongmt_args is True or False won't work.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BTW, I just updated the codes in #640, hopefully, to make the issue easier to understand.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So reading #640, does that mean that True/False inputs will eventually need to go here in build_arg_strings as well? It seems weird to have both kwargs_to_strings and build_arg_strings handling different pieces of Python to GMT logic.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does that mean that True/False inputs will eventually need to go here in build_arg_strings as well?

Yes.

It seems weird to have both kwargs_to_strings and build_arg_strings handling different pieces of Python to GMT logic.

Yes, as mentioned in #640, handling boolean arguments in kwargs_to_strings is incorrect, and we should fix #640 by moving the True/False conversion to build_arg_strings (but in a separate PR).

Copy link
Member

@weiji14 weiji14 Oct 8, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, this PR should be fine then for now. I'll approve it but you'll need to update the branch in order to merge.

else:
sorted_args.append("-{}{}".format(key, kwargs[key]))

Expand Down