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.

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

Convert booleans arguments in build_arg_string, not in kwargs_to_strings #1125

Merged
merged 11 commits into from Apr 5, 2021

Conversation

seisman
Copy link
Member

@seisman seisman commented Mar 26, 2021

Description of proposed changes

See #640 and #639 (the discussions and the fix) for context.

Changes in this PR:

  • 06bee27: Remove convert_bools parameter from kwargs_to_strings. So kwargs_to_strings won't convert boolean arguments.
  • b7210ff: Handle boolean arguments in the build_arg_string function.
  • 59a2b28: The boolean use_srtm parameter of load_earth_relief won't be converted to an empty string or removed anymore. Now it's safe to check if use_strm.
  • a9d167d: The autolabel parameter of subplot function can be either bool or str, but we had to check if autolabel is not None. It's not intuitive. Actually, we made a mistake in the initial version (4126c16) and then fixed it (eadb847).
  • aac61c2: Similarly, justify, angle, and font can be boolean, but we have to check if it's None. This commit also fixes the weird codes.

Fixes #640.

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.

Slash Commands

You can write slash commands (/command) in the first line of a comment to perform
specific operations. Supported slash commands are:

  • /format: automatically format and lint the code
  • /test-gmt-dev: run full tests on the latest GMT development version

@seisman seisman marked this pull request as ready for review March 26, 2021 06:44
@seisman seisman self-assigned this Mar 26, 2021
@seisman seisman added the maintenance Boring but important stuff for the core devs label Mar 26, 2021
@seisman seisman added this to the 0.4.0 milestone Mar 26, 2021
Copy link
Member

@weiji14 weiji14 left a comment

Choose a reason for hiding this comment

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

Just some minor comments for now. I think there's some room for improvement but will suggest more later (possibly after the weekend).

pygmt/helpers/utils.py Outdated Show resolved Hide resolved
pygmt/helpers/utils.py Outdated Show resolved Hide resolved
Co-authored-by: Wei Ji <23487320+weiji14@users.noreply.github.com>
Copy link
Member

@weiji14 weiji14 left a comment

Choose a reason for hiding this comment

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

A few suggestions, mostly on refactoring the if-then blocks. Feel free to disagree if it feels less readable to you.

pygmt/helpers/utils.py Outdated Show resolved Hide resolved
pygmt/helpers/utils.py Outdated Show resolved Hide resolved
pygmt/helpers/utils.py Outdated Show resolved Hide resolved
pygmt/helpers/utils.py Outdated Show resolved Hide resolved
pygmt/helpers/utils.py Outdated Show resolved Hide resolved
pygmt/src/subplot.py Outdated Show resolved Hide resolved
Comment on lines +180 to 184

if angle is True:
kwargs["F"] += "+a"
elif isinstance(angle, (int, float, str)):
kwargs["F"] += f"+a{str(angle)}"
Copy link
Member

Choose a reason for hiding this comment

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

Just thinking ahead to resolve #483 (allowing list inputs to angle/list/justify), we could simply use if angle (returns True for list/int/float/str inputs) like so:

Suggested change
if angle is True:
kwargs["F"] += "+a"
elif isinstance(angle, (int, float, str)):
kwargs["F"] += f"+a{str(angle)}"
if angle:
kwargs["F"] += "+a"
if isinstance(angle, (int, float, str)):
kwargs["F"] += str(angle)

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually, I was planing to work on #483, but I realized that the Figure.text() function needs more refactoring, so I opened PR #1121.

Personally, I prefer to make the changes to text.py as small as possible, and will address #483 after #1121 is merged.

Copy link
Member

Choose a reason for hiding this comment

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

Ok, as you wish then.

Comment on lines +186 to 189
if font is True:
kwargs["F"] += "+f"
elif isinstance(font, str):
kwargs["F"] += f"+f{font}"
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if font is True:
kwargs["F"] += "+f"
elif isinstance(font, str):
kwargs["F"] += f"+f{font}"
if font:
kwargs["F"] += "+f"
if isinstance(font, str):
kwargs["F"] += font

Comment on lines +191 to 194
if justify is True:
kwargs["F"] += "+j"
elif isinstance(justify, str):
kwargs["F"] += f"+j{justify}"
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if justify is True:
kwargs["F"] += "+j"
elif isinstance(justify, str):
kwargs["F"] += f"+j{justify}"
if justify:
kwargs["F"] += "+j"
if isinstance(justify, str):
kwargs["F"] += justify

Co-authored-by: Wei Ji <23487320+weiji14@users.noreply.github.com>
Copy link
Member

@weiji14 weiji14 left a comment

Choose a reason for hiding this comment

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

Thanks @seisman, good to finally close #640, and this change will open up a lot of new possibilities with PyGMT functions!

@seisman seisman merged commit 26d00e0 into master Apr 5, 2021
@seisman seisman deleted the fix-bools branch April 5, 2021 05:47
sixy6e pushed a commit to sixy6e/pygmt that referenced this pull request Dec 21, 2022
…ngs (GenericMappingTools#1125)

* Do not convert booleans in the kwargs_to_strings decorator
* Deal with boolean values in build_arg_string
* load_earth_relief no longer need the convert_bools=False hack
* Fix the weird codes in subplot autolabel
* Fix the weird codes in text

Co-authored-by: Wei Ji <23487320+weiji14@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
maintenance Boring but important stuff for the core devs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Decorator kwargs_to_strings is too aggressive when converting boolean arguments
2 participants