Session.virtualfile_in: Deprecate parameter 'required_data' to 'required' (will be removed in v0.20.0) #3931
Conversation
…red' (will be removed in v0.20.0)
seisman
force-pushed
the
virtualfile_in/required_data
branch
from
12e35e2 to
0c68337
Compare
seisman
added
deprecation
| if check_kind: | ||
| valid_kinds = ("file", "arg") if required_data is False else ("file",) | ||
| valid_kinds = ("file", "arg") if required is False else ("file",) |
There was a problem hiding this comment.
I feel that required by itself isn't a very descriptive parmeter name. This required_data parameter was added in #2493 to better handle optional virtual files (via the 'arg' kind). Would a name like optional_vfile (default to True) or something along those lines work? Then the check would be:
valid_kinds = ("file", "arg") if optional_vfile is False else ("file",)Or we could also just keep the required_data name as is? Thoughts?
There was a problem hiding this comment.
Would a name like
optional_vfile(default to True) or something along those lines work?
You meant optional_vfile default to False?
The data_kind takes two parameters data and required. Here, required_data is renamed to required to match the parameter name in that function. If we rename it to optional_vfile, then we should use data_kind(data, required=not optional_vfile).
There was a problem hiding this comment.
Ah yes, I meant optional_vfile=False by default, but that's probably not a great name either. On second thought, let's go with the required parameter name then. Using just required in data_kind makes sense because we're only passing in an argument to data, and I guess we do validate whether data/x/y are not None in virtualfile_in so there shouldn't be any ambiguity, but need to update the docstring a bit.
pygmt/src/text.py
Outdated
| check_kind="vector", | ||
| data=textfiles or data, | ||
| required_data=required_data, | ||
| check_kind="vector", data=textfiles or data, required=required_data |
There was a problem hiding this comment.
At L193 and L194 above. The required_data variable wasn't changed to required?
There was a problem hiding this comment.
Yes, required_data at L193-L194 is a local variable, and required is not a good name for it. Perhaps rename it to data_is_required?
Done in 4ea2ecb.
| if check_kind: | ||
| valid_kinds = ("file", "arg") if required_data is False else ("file",) | ||
| valid_kinds = ("file", "arg") if required is False else ("file",) |
There was a problem hiding this comment.
Ah yes, I meant optional_vfile=False by default, but that's probably not a great name either. On second thought, let's go with the required parameter name then. Using just required in data_kind makes sense because we're only passing in an argument to data, and I guess we do validate whether data/x/y are not None in virtualfile_in so there shouldn't be any ambiguity, but need to update the docstring a bit.
weiji14
left a comment
weiji14
left a comment
There was a problem hiding this comment.
Just one other suggestion. Tests failing appear unrelated, was there an update to the earth_relief grids again?
pygmt/clib/session.py
Outdated
| mincols | ||
| Number of minimum required columns. Default is 2 (i.e. require x and y | ||
| columns). |
There was a problem hiding this comment.
Also move this docstring to below required.
Address #3836.