Fix cbin_file_path#3524
Conversation
| if cbin_file is None: | ||
| if cbin_file is not None: | ||
| warnings.warn( | ||
| "The `cbin_file` argument is deprecated, please use `cbin_file_path` instead", DeprecationWarning |
There was a problem hiding this comment.
Did we need a stack level for this? I forget?
| "folder_path": str(Path(folder_path).resolve()), | ||
| "load_sync_channel": load_sync_channel, | ||
| "cbin_file": str(Path(cbin_file).resolve()), | ||
| "cbin_file_path": str(Path(cbin_file_path).resolve()), |
There was a problem hiding this comment.
One more question because I forget the json mechanics. Is this breaking for trying to read the extractor or will this just work?
There was a problem hiding this comment.
No this is back compatible since we kept both arguments
zm711
left a comment
zm711
left a comment
There was a problem hiding this comment.
I wonder if it would be nice for provenance to explain why this PR worked. The changes here by themselves don't make sense without doing that the tools scan for "path" in the name. I guess in the future people can read my comment and the issue.
But as the user said this fixes it.
h-mayorquin
left a comment
h-mayorquin
left a comment
There was a problem hiding this comment.
LGTM.
I would suggest adding the date on the deprecation warnings so we eventually remove them. and a minor typing thing on the docstring.
| if cbin_file is None: | ||
| if cbin_file is not None: | ||
| warnings.warn( | ||
| "The `cbin_file` argument is deprecated, please use `cbin_file_path` instead", |
There was a problem hiding this comment.
Maybe let's say when?
There was a problem hiding this comment.
unfortunately I think we'll have to keep this forever because of back-compatibility...
Co-authored-by: Heberto Mayorquin <h.mayorquin@gmail.com>
3 participants
Fixes #3523