Skip to content

Conversation

@seisman
Copy link
Member

@seisman seisman commented May 27, 2025

This PR extends the private _parse_sequence function in grdclip to a more general function sequence_join, which can join a 1-D or 2-D sequence by a separator.

@seisman seisman force-pushed the sequence_join branch 3 times, most recently from 49fa01a to 9ee2a21 Compare May 27, 2025 08:38
@seisman seisman added enhancement Improving an existing feature needs review This PR has higher priority and needs review. labels May 27, 2025
@seisman seisman added this to the 0.16.0 milestone May 27, 2025
@seisman seisman requested a review from Copilot June 3, 2025 01:32
Copy link
Contributor

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 PR introduces a new helper function, sequence_join, to join 1-D or 2-D sequences with a specified separator and replaces the older _parse_sequence calls in grdclip.

  • Replaces _parse_sequence with sequence_join in grdclip.
  • Adds the sequence_join implementation in pygmt/helpers/utils.py with extensive docstrings and examples.
  • Updates pygmt/helpers/init.py to export the new helper function.

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

File Description
pygmt/src/grdclip.py Updated to use the new sequence_join helper for joining sequences.
pygmt/helpers/utils.py Added the implementation of sequence_join with parameter validations.
pygmt/helpers/init.py Exported the newly added sequence_join for module-wide usage.

)


def sequence_join(
Copy link
Member

Choose a reason for hiding this comment

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

Keep this as a private function?

Suggested change
def sequence_join(
def _sequence_join(

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 function is used in grdclip.py and will likely be used in many other functions. So, I'd prefer to make it public.

Copy link
Member

Choose a reason for hiding this comment

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

Ok, not strong opinions, just worried we might need to refactor it again in the future (which requires more caution for public-facing functions).

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
@seisman seisman removed the needs review This PR has higher priority and needs review. label Jun 3, 2025
@seisman seisman merged commit 438edce into main Jun 3, 2025
22 of 24 checks passed
@seisman seisman deleted the sequence_join branch June 3, 2025 02:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement Improving an existing feature

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants