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

Add sequence_to_ctypes_array to convert a sequence to a ctypes array #3136

Merged
merged 6 commits into from
Mar 26, 2024

Conversation

seisman
Copy link
Member

@seisman seisman commented Mar 23, 2024

Description of proposed changes

The kwargs_to_ctypes_array function was one of the oldest functions in PyGMT. It was first added in PR #55 (It's 2017! Seven years ago!) and has not changed since then.

The function converts a sequence (like a list or a tuple) into a ctypes array, but the sequence must be the argument of a keyword dictionary, so it's called like this:

dim = kwargs_to_ctypes_array("dim", kwargs, ctp.c_uint64 * 4)

but why not just have a more general function sequence_to_ctypes_array, which can be called like:

dim = sequence_to_ctypes_array(kwargs.get("dim"), ctp.c_uint64 * 4)

The new function also works if dim is not a keyword argument:

dim = sequence_to_ctypes_array(dim, ctp.c_uint64 * 4)

This PR removes the old kwargs_to_ctypes_array function and adds the new sequence_to_ctypes_array function. It's defined differently and should be called like:

dim = sequence_to_ctypes_array(dim, ctp.c_uint64, 4) 

which I think is more readable.

The Session.create_data method is also slightly refactored because the kwargs parameter makes little sense in this method.

PR #3137 adds a similar function that converts a sequence of strings into a ctypes array. Better to review these two PRs back-to-back.

@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 Mar 23, 2024
@seisman seisman added this to the 0.12.0 milestone Mar 23, 2024
@seisman seisman removed the needs review This PR has higher priority and needs review. label Mar 23, 2024
@seisman seisman marked this pull request as draft March 23, 2024 08:16
@seisman seisman marked this pull request as ready for review March 23, 2024 08:52
@seisman seisman added needs review This PR has higher priority and needs review. run/benchmark Trigger the benchmark workflow in PRs and removed skip-changelog Skip adding Pull Request to changelog run/benchmark Trigger the benchmark workflow in PRs labels Mar 23, 2024
@seisman seisman changed the title Add a function sequence_to_ctypes_array to convert a sequence to a ctypes array Add sequence_to_ctypes_array to convert a sequence to a ctypes array Mar 23, 2024
@seisman seisman 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 Mar 25, 2024
@seisman seisman merged commit 754a52f into main Mar 26, 2024
21 checks passed
@seisman seisman deleted the sequence_to_ctypes_array branch March 26, 2024 01:25
@seisman seisman removed the final review call This PR requires final review and approval from a second reviewer label Mar 26, 2024
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.

None yet

3 participants