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 function build_arg_list for building arguments list from keyword dictionaries #3149

Merged
merged 7 commits into from
Apr 8, 2024

Conversation

seisman
Copy link
Member

@seisman seisman commented Mar 31, 2024

This PR contains the changes of commit 1149b37 in PR #3132.

In this PR, a new utility function build_arg_list function is added. The new function is modified from the build_arg_string function and it differs from the build_arg_string function in the following aspects:

  1. returns a list of arguments, instead of a single argument string
  2. no hacky solutions for handling whitespaces
  3. Only apply the non_ascii_to_octal function to the single-letter option arguments, not to the configuration arguments and input/output files
  4. Doctests are also simplified.

The goal is to replace build_arg_string with build_arg_list in all module wrappers and deprecate/remove build_arg_string in future releases.

PR #3132 contains more changes that we will apply after this PR is merged.

@seisman seisman added enhancement Improving an existing feature needs review This PR has higher priority and needs review. labels Mar 31, 2024
@seisman seisman added this to the 0.12.0 milestone Mar 31, 2024
seisman and others added 2 commits April 1, 2024 07:50
Co-authored-by: Yvonne Fröhlich <94163266+yvonnefroehlich@users.noreply.github.com>
@seisman seisman requested a review from a team April 2, 2024 01:25
@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 Apr 7, 2024
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.

Code logic ok, just some suggestions related to the type hints.

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
seisman and others added 2 commits April 8, 2024 09:53
Co-authored-by: Wei Ji <23487320+weiji14@users.noreply.github.com>
@seisman seisman enabled auto-merge (squash) April 8, 2024 03:05
@seisman seisman merged commit 79cda63 into main Apr 8, 2024
19 checks passed
@seisman seisman deleted the build_arg_list branch April 8, 2024 03:08
@seisman seisman removed the final review call This PR requires final review and approval from a second reviewer label Apr 8, 2024
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.

None yet

4 participants