[MRG] Auto generates deprecation for sklearn.utils.mocking #15071
Conversation
If that also works when changes are added to |
|
||
|
||
# Adds files that will be deprecated | ||
def _adds_deprecated_submodules(): |
NicolasHug
Sep 23, 2019
Member
def _adds_deprecated_submodules(): | |
def _add_deprecated_submodules(): |
Also it would need a short description (remaning files + creating new file to keep the import)
import os | ||
|
||
from sklearn._build_utils import maybe_cythonize_extensions | ||
|
||
|
||
_DEPRECATED_MODULES = { |
NicolasHug
Sep 23, 2019
Member
Maybe put this whole logic in another file so we can just mark it to be removed in 0.24
Will this work at conclusion?
|
At conclusion? |
At conclusion of the deprecation, i.e. 0.24
|
At conclusion, the code to generate the file will be removed. In this case, |
Of course. So the rename here preserved history as long as no file is
created with the old name.
|
The file is created, it's just git ignored. That's the main difference with the other strategy |
The file is not in the repo. It is only created during setup. |
@thomasjpfan can you check that this doesn't create complex merge conflict when:
|
@NicolasHug As a demo, I used
Looks like there are no merge conflicts. |
It looks like the thomasjpfan#14 PR is directly modifying We should instead try to make changes to |
thomasjpfan#15 there is a merge conflict. Running |
Having tested this locally it seems that it does create merge conflicts, but they are really trivial to solve now (since the I'm +1 |
ping @adrinjalali @glemaitre @rth I think this would enable the large scale deprecations that we've been trying |
Nits about names but LGMT |
|
||
|
||
_DEPRECATED_MODULES = { | ||
# TODO: Remove in 0.24 |
""" | ||
|
||
|
||
def _add_deprecated_submodules(): |
NicolasHug
Sep 25, 2019
Member
def _add_deprecated_submodules(): | |
def _create_deprecated_modules_files(): |
?
('_mocking', 'sklearn.utils.mocking', 'sklearn.utils') | ||
} | ||
|
||
_DEPRECATE_TEMPLATE = """from .{module} import * # noqa |
NicolasHug
Sep 25, 2019
Member
_DEPRECATE_TEMPLATE = """from .{module} import * # noqa | |
_FILE_CONTENT_TEMPLATE = """from .{module} import * # noqa |
|
||
|
||
_DEPRECATED_MODULES = { | ||
# TODO: Remove in 0.24 |
NicolasHug
Sep 25, 2019
Member
Please add comment indicating that the 3tuple is (underscored_filed, deprecated_path, correct_path)
Don't ping me on the commit comments please ^^ |
from pathlib import Path | ||
|
||
# This is a set of 3-tuples consisting of | ||
# (new_module_path, deprecated_path, correct_path) |
NicolasHug
Sep 25, 2019
Member
# (new_module_path, deprecated_path, correct_path) | |
# (new_module_name, deprecated_path, correct_import_path) |
I didn’t notice that it pings. I’ll remove the @ next time. |
I think we should also remove those files in |
deprecated_parts = deprecated_path.split(".") | ||
deprecated_parts[-1] = deprecated_parts[-1] + ".py" | ||
|
||
with Path(*deprecated_parts).open('w') as f: |
thomasjpfan
Sep 27, 2019
Author
Member
I do not see any side effect of this. This can be good if we decide to adjust the template?
Was wondering, are the files gonna be shipped with the wheels?? |
Currently, I think it is shipped with the wheels. Is that an issue? |
It's an issue if they're not |
Confirmed it is in the wheel. |
LGTM, thanks @thomasjpfan |
Please add to what's new under sklearn.utils |
dba753a
into
scikit-learn:master
Thanks @thomasjpfan @adrinjalali @glemaitre let's update our PRs :) |
Just getting back to these PRs. This solution looks nice :) |
Reference Issues/PRs
Alternative to #15039
What does this implement/fix? Explain your changes.
Autogenerates deprecation file to preserve git glame.
CC @NicolasHug