Skip to content
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

Incorrect deprecation warning when calling civis_file_to_table #360

Closed
jsfalk opened this issue Feb 12, 2020 · 4 comments · Fixed by #393
Closed

Incorrect deprecation warning when calling civis_file_to_table #360

jsfalk opened this issue Feb 12, 2020 · 4 comments · Fixed by #393
Labels

Comments

@jsfalk
Copy link
Contributor

jsfalk commented Feb 12, 2020

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.

@deprecate_param('v2.0.0', 'file_id')

@jsfalk jsfalk added the bug label Feb 12, 2020
@stephen-hoover
Copy link

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
Copy link
Contributor

mheilman commented Mar 27, 2020

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: #328

@stephen-hoover
Copy link

@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
Copy link
Contributor

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.

stephen-hoover pushed a commit to stephen-hoover/civis-python that referenced this issue May 12, 2020
There was no way for users to avoid this deprecation warning when calling `civis.io.civis_file_to_table`, and it was also triggered by other functions which called `civis_file_to_table` (such as `dataframe_to_civis`). The original thought had been to replace the singular `file_id` parameter with a plural `file_ids` parameter in v2, but per discussion in civisanalytics#360, the singular will be acceptable.
stephen-hoover pushed a commit that referenced this issue May 13, 2020
There was no way for users to avoid this deprecation warning when calling `civis.io.civis_file_to_table`, and it was also triggered by other functions which called `civis_file_to_table` (such as `dataframe_to_civis`). The original thought had been to replace the singular `file_id` parameter with a plural `file_ids` parameter in v2, but per discussion in #360, the singular will be acceptable.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants