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

DEP Remove file_id deprecation warning #393

Conversation

stephen-hoover
Copy link

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.

Closes #360.

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 stephen-hoover changed the title MAINT Remove file_id deprecation warning DEP Remove file_id deprecation warning May 12, 2020
New flake8 versions introduce new tests which the code might not satisfy.
@stephen-hoover
Copy link
Author

@mheilman , flake8 v3.8 came out yesterday, and there's new checks which civis-python fails:

civis/run_joblib_func.py:108:19: F523 '...'.format(...) has unused arguments at position(s): 1
civis/ml/_model.py:199:30: E741 ambiguous variable name 'l'
civis/ml/_model.py:205:48: E741 ambiguous variable name 'l'
civis/ml/tests/test_model.py:377:43: E741 ambiguous variable name 'l'
civis/tests/test_io.py:1094:25: E741 ambiguous variable name 'l'
civis/tests/test_io.py:1115:34: E741 ambiguous variable name 'l'

What's your preferred resolution (cap flake8 version, fix flake8 errors, ignore flake8 errors), and should it be in this PR or another? Those things look relatively easy to fix, and at least the unused format parameter should be fixed.

@mheilman
Copy link
Contributor

mheilman commented May 12, 2020

I made a separate PR to deal with the flake8 stuff: #394

(It seemed best to make things work with flake8 3.8 since one of the things it flagged was a minor bug in "logging" in the joblib runner)

Copy link
Contributor

@mheilman mheilman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@stephen-hoover stephen-hoover merged commit 0fd0820 into civisanalytics:master May 13, 2020
@stephen-hoover stephen-hoover deleted the remove-files-to-table-deprecation branch May 13, 2020 16:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Incorrect deprecation warning when calling civis_file_to_table
2 participants