-
-
Notifications
You must be signed in to change notification settings - Fork 19.3k
Make kwargs explicit in put, append #29957
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
Conversation
simonjayhawkins
left a comment
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.
@jbrockmendel lgtm couple of suggestions, not blockers.
| raise TypeError(f"invalid HDFStore format specified [{format}]") | ||
|
|
||
| return kwargs | ||
| return format |
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.
could just return in try block and remove intermediate variable?
pandas/core/generic.py
Outdated
| complib: Optional[str] = None, | ||
| append: bool_t = False, | ||
| format: Optional[str] = None, | ||
| min_itemsize=None, |
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.
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.
i like this idea, will do in a dedicated PR
these are likely ok. The reason for extra keywords is mainly on opening of the store itself, where PyTables passes thru keywords (e.g. to specify a different kind of backend: https://github.com/PyTables/PyTables/blob/master/tables/file.py#L219) |
…n-pytables-kwargs4
To be clear, I'm referring to keywords that we have established are accepted by _write_to_group, i.e. not ones being passed to HDFStore. |
pandas/core/generic.py
Outdated
| complib: Optional[str] = None, | ||
| append: bool_t = False, | ||
| format: Optional[str] = None, | ||
| min_itemsize=None, |
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.
this is Union[Optional[int], Dict[str, int]]
pandas/core/generic.py
Outdated
| append: bool_t = False, | ||
| format: Optional[str] = None, | ||
| min_itemsize=None, | ||
| data_columns=None, |
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.
Optional[List[str]]
pandas/core/generic.py
Outdated
| See the errors argument for :func:`open` for a full list | ||
| of options. | ||
| encoding : str, default "UTF-8" | ||
| min_itemsize : dict, optional |
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.
see above
pandas/io/pytables.py
Outdated
| data_columns=data_columns, | ||
| errors=errors, | ||
| encoding=encoding, | ||
| **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.
yeah ok with removing the kwargs
then i think ok to remove |
|
Updated:
|
|
updated+green (ex an unrelated jinja2 problem ill work on in the morning) |
…n-pytables-kwargs4
|
thanks |
Made possible by #29951, progress towards #20903
@jreback input needed on whether certain not-covered-in-tests cases should be considered possible. See that both
putandappendstill contain**kwargs. In all extant tests, kwargs is always empty. But there are remaining keywords that write_to_group (which both put and append call) accepts that are not in the put/append signatures.So the question is: is there a reason why put/append would accept different subsets of the writing keywords? If not, they should both have the same explicit kwargs as write_to_group.
Once this is pinned down, we can get explicit all the way up the stack to NDFrame.to_hdf