Add Deprecated APIs Depended on by Legacy Tools#70
Add Deprecated APIs Depended on by Legacy Tools#70Alek Binion (biniona) merged 30 commits intomasterfrom
Conversation
Alek Binion (biniona)
changed the title
Alek Binion (biniona)
changed the title
|
|
||
| .. deprecated:: 1.0 | ||
| The _name and _start parameters are deprecated and should not be used in new code. | ||
| To replace _start, use a context manager instead. There is no replacement for _name. |
There was a problem hiding this comment.
I plan to add the context manager API as the next PR
| return getattr(module, var_name) | ||
|
|
||
|
|
||
| class _RegistryWrapper(Generic[T]): |
There was a problem hiding this comment.
I made this private for the following reasons:
- It always should have been private - and it is not currently depended on
- It lets us not mark the start/close methods as deprecated
| The provided mapping should contain general configuration that can | ||
| be accessed using the inject.config decorator. | ||
|
|
||
| .. deprecated:: 1.0 |
There was a problem hiding this comment.
I'm not familiar with this pydoc syntax, and having some trouble finding a description of it — could you send me a link to the doc?
There was a problem hiding this comment.
I believe this is a restructured text admonition - which could be rendered using a tool like Sphinx.
I am not aware of a standard way to deprecate Python code for Python<3.13 (@deprecated) - so Claude's suggestion to use this seemed alright to me.
The only test I did for this was checking how it rendered when hovering over the function/method name in VSCode:
Let me know if this seems incorrect!
minject/inject.py
Outdated
|
|
||
| # we must export this from minject to provide backwards | ||
| # compatability to a legacy system | ||
| __all__ = ["_get_meta"] |
There was a problem hiding this comment.
If I understand PEP-8 correctly, __all__ should include all of the public identifiers, not just exceptional additions.
There was a problem hiding this comment.
Added all public methods in the inject module to this all` list.
minject/registry.py
Outdated
| """ | ||
| for key in self._autostart_candidates(): | ||
| LOG.debug("autostarting %s", key) | ||
| self[key] # pylint: disable=pointless-statement |
There was a problem hiding this comment.
(nit) I think _ = self[key] would be a cleaner way to disable this warning?
minject/registry.py
Outdated
| ) | ||
|
|
||
| def _autostart_candidates(self) -> Iterable[RegistryKey]: | ||
| registry_config: Mapping[str, Any] = self.config.get("registry") |
There was a problem hiding this comment.
Should this be Optional[Mapping[str, Any]]? The if registry_config: below suggests that this could be None — if it were merely an empty Mapping, that condition would be redundant.
Perhaps:
registry_config: Optional[Mapping[str, Any]] = self.config.get("registry")
if registry_config is not None:
if (autostart := registry_config.get("autostart")) is not None:
return (_resolve_import(value) for value in autostart)
return ()
minject/registry.py
Outdated
| self._by_meta: Dict[RegistryMetadata, RegistryWrapper] = {} | ||
| self._by_name: Dict[str, RegistryWrapper] = {} | ||
| self._by_iface: Dict[type, List[RegistryWrapper]] = {} | ||
| self._objects: List[_RegistryWrapper] = [] |
There was a problem hiding this comment.
_RegistryWrapper is parameterized on T, so these fields should use _RegistryWrapper[Any] instead of just _RegistryWrapper. (I fixed these in the internal repo in https://github.com/duolingo/python-duolingo-base/pull/667 — you might be able to have Cursor backport that patch.)
This also suggests that we need to improve the type-checking in this repo's CI configuration!
There was a problem hiding this comment.
Great points. I'll do the following:
- Fix this type error
- Port #667 to this PR where applicable
- Update
mypycheck on this repo to run--strict
Let me know if you have any concerns with this approach
tests/test_registry.py
Outdated
| self.assertEqual(count, query_per_class) | ||
|
|
||
| def test_bind_name_parameter(self) -> None: | ||
| """Test that the _name parameter of inject.bind() works correctly and emits a deprecation warning.""" |
There was a problem hiding this comment.
I don't think this is checking the “and emits a deprecation warning” part?
minject/inject.py
Outdated
|
|
||
| def start_method(cls, method): | ||
| # type: (Type[T], Callable[[T], None]) -> None | ||
| """Function to bind a registry start function for a class. |
There was a problem hiding this comment.
A bit more detail in the documentation here would be helpful — if there is already a start function bound, what is the intended behavior? I can think of three reasonable-ish options:
- The call to
start_methodthrows something like anAssertionErrororValueError, indicating erroneous usage. - The new method is registered to be called after any previously-registered methods.
- The new method replaces any previously-registered methods. (But then we probably also need some API for reporting the previous methods, so that they can be chained properly?)
The implementation here seems to be (3), but I don't see an API or examples for how do properly chain calls. 🤔
Would it be feasible to switch to (1) instead, or do we think that will break existing usage?
There was a problem hiding this comment.
Gread idea. I checked the start_method usage and AFAICT it is not paired with a @<decorator>(_start=...) usage.
We should be able to update the API to 1.
Added a test to check this as well.
|
Waiting on the final go ahead from Matt McHenry (@mmchenry-duolingo) to merge |
Makes sense to me; code looks good |
3 participants
This PR is essentially a revert of the following four commits, being careful to only re-add features that are currently depended on:
This PR adds the following deprecated APIs to allow the Public version of this project to fully replace the private version.
The depreacted APIs we are adding are as follows:
registry.autostartnamespace in a config file_nameand_startparameters back into theinject.bind()+inject.define()functions, allowing you to rename a binding in the registry and define a method to call when the binding is first initialized.Registry.startback instart_method+close_methoddecorators back inregistry.config.from_dictmethod back inNONE OF THESE APIS SHOULD BE USED BY ANY NEW CODE!!!!
I am not adding the following deprecated APIs back in as they do not appear to be used:
inject.nameconfig.RegistrySubConfigconfig.InternalRegistryConfigregistry.by_classandregistry.by_namenamespacesSemi-Related Change - I am also making the
RegistryWrapperclass private. It is not referenced outside the library and as far as I can tell it has no reason to be. This also allows us to not mark aspects of the class as deprecated, as it is private.