[MRG] Solve part of 'Remove fixes/compat for Python 2' #11991 #15231
Conversation
The function
sklearn.datasets.base._pkl_filepath
is a compatibility function for Python 2 support.
The function expects path components as *args and constructs and returns the path to a pickel from it.
It modifies the last path component as follows:
- For Python 3, it adds a suffix ('_py3' by default, or the value of the kwarg 'py3_suffix' if specified).
- For Python 2, it does nothing .
It then concatenates its arguments using os.path.join and returns the
result.
If Python 2 support is to be deprecated, we can
- remove the function _pkl_filepath and
- replace all calls to it by direct calls to os.path.join.
|
I am quite sure that my changes should not affect the code coverage of the code sections reported by the tool. Could this be a glitch on the codecov side? |
|
The issue is that users may still have Python 2 pickles in their cache. So if we want to remove that function, we need to manually change all the file paths that use it to include the suffix. |
Well, you removed a function which was covered in tests so the total coverage decreased slightly, that's fine, don't worry about it. |
|
Will revert changes in new PR. |
|
@sbehren better to add them in this one. You can just push additional commits and change the PR title as need. It make it much easier for reviewers to follow what is going on. |
Thanks for the info, will do! |
|
I force-pushed my changes to my old branch. This prevents me from reopening this MR. I did not know this, sorry @rth |
Hmm, I didn't know that either. Force pushing in an open PR works fine (though it's better to avoid force pushing and adding commits instead in any case). Please ping me in the other PR that you open. |
Reference Issues/PRs
Solves part of #11991
What does this implement/fix? Explain your changes.
The function
sklearn.datasets.base._pkl_filepath
is a compatibility function for Python 2 support.
The function expects path components as *args and constructs and returns the path to a pickel from it.
It modifies the last path component as follows:
It then concatenates its arguments using os.path.join and returns the
result.
If Python 2 support is to be deprecated, we can
as done in this MR.
Any other comments?