Skip to content

Conversation

seisman
Copy link
Member

@seisman seisman commented Jun 19, 2025

See #3981 for the bug report. Syntax like frame=[True, "+tThe Caribbean Sea"] is not intuitive but is still valid. This PR fixes the issue by checking if a sequence contains True.

Other values like False/None are not checked, since they make no sense, e.g., frame=[False, "+tThe Caribbean Sea"]

Closes #3981.

@seisman seisman added this to the 0.17.0 milestone Jun 19, 2025
@seisman seisman added bug Something isn't working needs review This PR has higher priority and needs review. labels Jun 19, 2025
@seisman seisman changed the title Fix the bug that passing a sequence that contains True to a parameter Fix the bug when passing a sequence that contains True to a parameter Jun 19, 2025
@seisman seisman requested a review from Copilot June 20, 2025 02:58
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This pull request fixes the bug when passing a sequence containing True to a parameter by modifying the argument list construction in pygmt.

  • Adds a check inside the iterable processing to handle True values correctly.
  • Updates the behavior in build_arg_list to distinguish between a bare True and other iterable elements.
Comments suppressed due to low confidence (1)

pygmt/helpers/utils.py:570

  • [nitpick] Consider adding an inline comment to explain why the comprehension conditionally checks for True values within the iterable, for future maintainers.
            gmt_args.extend(

@seisman seisman requested a review from a team June 23, 2025 09:34
Test for https://github.com/GenericMappingTools/pygmt/issues/3981.
"""
fig = Figure()
fig.basemap(region=[0, 10, 0, 10], projection="X10c", frame=[True, "WSen"])
Copy link
Member

Choose a reason for hiding this comment

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

This translates to gmt basemap -R0/10/0/10 -JX10c -B -BWSen -pdf temp in GMT, which seems a little weird, but I see the output is different from gmt basemap -R0/10/0/10 -JX10c -BWSen -pdf temp, so ok I guess.

@seisman seisman merged commit 0ef29f8 into main Jun 24, 2025
23 of 27 checks passed
@seisman seisman deleted the bug/bool-arg branch June 24, 2025 01:17
@seisman seisman removed the needs review This PR has higher priority and needs review. label Jun 24, 2025
@weiji14
Copy link
Member

weiji14 commented Jun 24, 2025

DVC test failed because they're not shipping binaries anymore? See bug report at iterative/setup-dvc#69.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Pass a sequence that contains bool and string to a parameter
2 participants