Disallow passing arguments like -XNone to GMT

[seisman]

Description of proposed changes

Arguments like -XNone are invalid for GMT.

This PR updates the function build_arg_string and checks if any value is None.
If yes, remove the argument.

Limitations:

Invalid arguments like {'X': [None, None, None]} are still not caught and
converted to -XNone -XNone -XNone, but it should be uncommon and more likely
a user error.

Fixes #380.

Reminders

• Run make format and make check to make sure the code follows the style guide.
• Add tests for new features or tests that would have caught the bug that you're fixing.
• Add new public functions/methods/classes to doc/api/index.rst.
• Write detailed docstrings for all functions/methods.
• If adding new functionality, add an example to docstrings or tutorials.

[seisman]

@MarkWieczorek Please see if this PR works for you.

[weiji14]

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

[seisman]

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

[weiji14]

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.

[seisman]

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.

[seisman]

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

[weiji14]

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.

[seisman]

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).

[weiji14]

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.

[weiji14]

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.