search

civisanalytics / civis-python

Civis API Python Client
BSD 3-Clause "New" or "Revised" License
34 stars 26 forks source link

Incorrect deprecation warning when calling `civis_file_to_table` #360

Closed jsfalk closed 4 years ago

jsfalk commented 4 years ago

file_id is deprecated only as a named parameter, but the @deprecate_param decorator issues a warning whether file_id is used as a named parameter or a positional parameter. This also causes invalid warnings when using dataframe_to_civis or csv_to_civis.

https://github.com/civisanalytics/civis-python/blob/94da3a01ce842a1c42fc585183af2668e3bf928c/civis/io/_tables.py#L877

stephen-hoover commented 4 years ago

This warning has been popping up for me too. My suggestion is that you move file_id to the end of the parameter list and replace the first positional argument with whatever you want the new name of the parameter to be. Anyone calling the function with file_id explicitly named will get the deprecation warning, but there will be no warning for using the argument positionally. Users will also be able to start using the new parameter name if they want to pass parameters by name.

mheilman commented 4 years ago

I'd be inclined to just remove the deprecation warning and leave file_id as is for now.

It seems not uncommon for python libraries to have arguments be singular when a single value or list could be provided (e.g., dtype for pandas.read_csv).

Also, if we're concerned about file_id being singular, then we'd probably want to rename the function to civis_files_to_table also (or add a new one), rather than, say, have civis_file_to_table take file_ids.

The PR that I believe introduced this issue: https://github.com/civisanalytics/civis-python/pull/328

stephen-hoover commented 4 years ago

@mheilman , would you accept a PR which removed the deprecation warning and left the code unchanged? Or would you like to continue to move toward a file_ids parameter? I could also make a PR for that, along the lines of my last comment.

mheilman commented 4 years ago

Yeah, I think we should just remove the deprecation warning. I'd be happy to accept that PR (or let me know if you'd like me to make the PR). Thanks for reminding me about this.