[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
Suggested change
def _adds_deprecated_submodules():
def _add_deprecated_submodules():
Also it would need a short description (remaning files + creating new file to keep the import)
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
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 |
NicolasHug
Sep 25, 2019
Member
The entire file should be removed in 0.24 right? (ideally)
The entire file should be removed in 0.24 right? (ideally)
thomasjpfan
Sep 25, 2019
Author
Member
That is the ideal case. ;)
That is the ideal case. ;)
""" | ||
|
||
|
||
def _add_deprecated_submodules(): |
NicolasHug
Sep 25, 2019
Member
Suggested change
def _add_deprecated_submodules():
def _create_deprecated_modules_files():
?
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
Suggested change
_DEPRECATE_TEMPLATE = """from .{module} import * # noqa
_FILE_CONTENT_TEMPLATE = """from .{module} import * # noqa
_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)
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
Suggested change
# (new_module_path, deprecated_path, correct_path)
# (new_module_name, deprecated_path, correct_import_path)
# (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: |
adrinjalali
Sep 26, 2019
Member
we don't mind potentially overriding those files?
we don't mind potentially overriding those files?
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?
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