-
Notifications
You must be signed in to change notification settings - Fork 220
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
pygmt.which: Fix the bug when passing multiple files #2726
Conversation
|
||
|
||
def test_which_multiple(): | ||
""" | ||
Make sure `which` returns file paths for multiple @files correctly. | ||
""" | ||
filenames = ["ridge.txt", "tut_ship.xyz"] | ||
cached_files = which(fname=[f"@{fname}" for fname in filenames], download="c") | ||
cached_files = which([f"@{fname}" for fname in filenames], download="c") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now we can pass a list of files without having to use fname=
use_alias, | ||
) | ||
|
||
|
||
@fmt_docstring | ||
@use_alias(G="download", V="verbose") | ||
@kwargs_to_strings(fname="sequence_space") | ||
def which(fname, **kwargs): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reading up on https://peps.python.org/pep-0570/#positional-or-keyword-arguments and https://stackoverflow.com/questions/9450656/positional-argument-vs-keyword-argument (see also Max's previous comment at #731 (comment)), should we explicitly mark fname
as positional_or_keyword like this (unsure if syntax is ok)?
def which(fname, **kwargs): | |
def which(/, fname, *, **kwargs): |
Or just stick with the current one (which works also I think).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Better to open a separate issue so that we can have more discussions about the behavior and also have consistent function definitions in the whole project.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On second thought, maybe we should enforce using keyword-only arguments like so:
def which(fname, **kwargs): | |
def which(*, fname, **kwargs): |
Then the gmtwhich [ERROR]
you mentioned at #2361 (comment) would turn into a slightly more useful error:
---------------------------------------------------------------------------
TypeError Traceback (most recent call last)
Cell In[4], line 1
----> 1 cached_files = which([f"@{fname}" for fname in filenames], download="c")
File ~/pygmt/pygmt/helpers/decorators.py:598, in use_alias.<locals>.alias_decorator.<locals>.new_module(*args, **kwargs)
591 msg = (
592 "Parameters 'Y' and 'yshift' are deprecated since v0.8.0. "
593 "and will be removed in v0.12.0. "
594 "Use Figure.shift_origin(yshift=...) instead."
595 )
596 warnings.warn(msg, category=SyntaxWarning, stacklevel=2)
--> 598 return module_func(*args, **kwargs)
TypeError: which() takes 0 positional arguments but 1 was given
Well, actually not that useful (it doesn't say that the fname
keyword should be used. But something to consider - if we want to allow 1) positional_or_keyword (current), or 2) only keyword.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead, I prefer to def which(fname, *, **kwargs)
. It allows which("file.txt")
which is much simpler than which(fname="file.txt")
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, let's go with that. I'll approve this PR, but you can apply the change afterwards.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
def which(fname, *, **kwargs)
gives a syntax error:
'named arguments must follow bare *
I'll leave the function definition as it is now.
Co-authored-by: Wei Ji <[email protected]>
use_alias, | ||
) | ||
|
||
|
||
@fmt_docstring | ||
@use_alias(G="download", V="verbose") | ||
@kwargs_to_strings(fname="sequence_space") | ||
def which(fname, **kwargs): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, let's go with that. I'll approve this PR, but you can apply the change afterwards.
Description of proposed changes
Issue first reported in #2361 (comment). See that issue for context.
To reproduce the issue:
Address #2361.
Reminders
make format
andmake check
to make sure the code follows the style guide.doc/api/index.rst
.Slash Commands
You can write slash commands (
/command
) in the first line of a comment to performspecific operations. Supported slash commands are:
/format
: automatically format and lint the code/test-gmt-dev
: run full tests on the latest GMT development version