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

Figure.legend: Refactor to simplify the logic of checking legend specification #3437

Merged
merged 3 commits into from
Sep 12, 2024

Conversation

seisman
Copy link
Member

@seisman seisman commented Sep 11, 2024

Description of proposed changes

This PR contains a subset of changes in PR #3326. I open this PR so that PR #3326 can be smaller and easier for review.

This PR refactors the following codes:

    with Session() as lib:
        if spec is None:
            specfile = ""
        elif data_kind(spec) == "file" and not is_nonstr_iter(spec):
            # Is a file but not a list of files
            specfile = spec
        else:
            raise GMTInvalidInput(f"Unrecognized data type: {type(spec)}")
        lib.call_module(module="legend", args=build_arg_list(kwargs, infile=specfile))

to

    kind = data_kind(spec)
    if kind not in {"vectors", "file"}:  # kind="vectors" means spec is None
        raise GMTInvalidInput(f"Unrecognized data type: {type(spec)}")
    if kind == "file" and is_nonstr_iter(spec):
        raise GMTInvalidInput("Only one legend specification file is allowed.")

    with Session() as lib:
        lib.call_module(module="legend", args=build_arg_list(kwargs, infile=spec))

The pros are:

  • Raise exceptions if any before entering a session
  • Better error message if multiple legend files are given

@seisman seisman added maintenance Boring but important stuff for the core devs skip-changelog Skip adding Pull Request to changelog needs review This PR has higher priority and needs review. labels Sep 11, 2024
@seisman seisman added this to the 0.14.0 milestone Sep 11, 2024
@@ -75,12 +87,11 @@ def legend(self, spec=None, position="JTR+jTR+o0.2c", box="+gwhite+p1p", **kwarg
if kwargs.get("F") is None:
kwargs["F"] = box

kind = data_kind(spec)
if kind not in {"vectors", "file"}: # kind="vectors" means spec is None
Copy link
Member Author

Choose a reason for hiding this comment

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

This line can also be written as:

if spec is not None or kind != "file"

It's written in the current way so that this line can be extended to check "stringio" kind with minor changes (xref: #3326).

It's not straightforward to see why kind="vectors" means spec is None. That's also why I want to refactor the data_kind function in #3351.

pygmt/src/legend.py Outdated Show resolved Hide resolved
@michaelgrund michaelgrund added final review call This PR requires final review and approval from a second reviewer and removed needs review This PR has higher priority and needs review. labels Sep 12, 2024
Co-authored-by: Michael Grund <[email protected]>
@seisman seisman removed the final review call This PR requires final review and approval from a second reviewer label Sep 12, 2024
@seisman seisman merged commit 98e2f6a into main Sep 12, 2024
10 of 11 checks passed
@seisman seisman deleted the refactor/legend branch September 12, 2024 16:00
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 skip-changelog Skip adding Pull Request to changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants