Add Deprecated APIs Depended on by Legacy Tools #70

biniona · 2025-04-10T01:00:29Z

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:

Initialize classes specified in the registry.autostart namespace in a config file
Adding the _name and _start parameters back into the inject.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.
Adding Registry.start back in
Adding start_method + close_method decorators back in
Adding the registry.config.from_dict method back in

NONE 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.name
config.RegistrySubConfig
config.InternalRegistryConfig
Initializing classes using the registry.by_class and registry.by_name namespaces

Semi-Related Change - I am also making the RegistryWrapper class 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.

biniona · 2025-04-10T21:44:50Z

minject/inject.py

+
+    .. 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.


I plan to add the context manager API as the next PR

biniona · 2025-04-10T21:47:44Z

minject/registry.py

+    return getattr(module, var_name)
+
+
+class _RegistryWrapper(Generic[T]):


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

bcmills · 2025-04-11T15:00:02Z

minject/config.py

        """Configure the registry from a dictionary-like mapping.
        The provided mapping should contain general configuration that can
        be accessed using the inject.config decorator.

+        .. deprecated:: 1.0


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?

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!

bcmills · 2025-04-11T15:09:13Z

minject/inject.py

+
+# we must export this from minject to provide backwards
+# compatability to a legacy system
+__all__ = ["_get_meta"]


If I understand PEP-8 correctly, __all__ should include all of the public identifiers, not just exceptional additions.

Added all public methods in the inject module to this all` list.

bcmills · 2025-04-11T15:15:03Z

minject/registry.py

+        """
+        for key in self._autostart_candidates():
+            LOG.debug("autostarting %s", key)
+            self[key]  # pylint: disable=pointless-statement


(nit) I think _ = self[key] would be a cleaner way to disable this warning?

bcmills · 2025-04-11T15:20:03Z

minject/registry.py

@@ -115,9 +128,41 @@ async def _push_async_context(self, key: Any) -> Any:
                ).strip()
            )

+    def _autostart_candidates(self) -> Iterable[RegistryKey]:
+        registry_config: Mapping[str, Any] = self.config.get("registry")


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 ()

bcmills · 2025-04-11T15:24:55Z

minject/registry.py

-        self._by_meta: Dict[RegistryMetadata, RegistryWrapper] = {}
-        self._by_name: Dict[str, RegistryWrapper] = {}
-        self._by_iface: Dict[type, List[RegistryWrapper]] = {}
+        self._objects: List[_RegistryWrapper] = []


_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!

Great points. I'll do the following:

Fix this type error

Port #667 to this PR where applicable

Update mypy check on this repo to run --strict

Let me know if you have any concerns with this approach

bcmills · 2025-04-11T15:27:25Z

tests/test_registry.py

@@ -476,6 +495,20 @@ def lazy_load_object(i):
        for count in Counter(map(id, results)).values():
            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."""


I don't think this is checking the “and emits a deprecation warning” part?

bcmills · 2025-04-11T15:34:36Z

minject/inject.py

@@ -96,12 +123,49 @@ def async_context(cls: Type[T_async_context]) -> Type[T_async_context]:
    return cls


+def start_method(cls, method):
+    # type: (Type[T], Callable[[T], None]) -> None
+    """Function to bind a registry start function for a class.


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_method throws something like an AssertionError or ValueError, 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?

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.

biniona · 2025-04-11T19:31:28Z

Waiting on the final go ahead from @mmchenry-duolingo to merge

biniona added 4 commits April 9, 2025 20:46

reverse b57ff26

66b5d82

reverse 6618e02

4450aa6

reverse 27a3977

b3571e6

reverse 05e005b

1658136

biniona mentioned this pull request Apr 10, 2025

Support Legacy APIs #69

Closed

biniona added 4 commits April 9, 2025 21:08

import concatenate

eca5b1e

fix import

fa5389e

re add config api

e6fad6e

type error

2bf14f4

biniona changed the title ~~Add Deprecated APIs Depended on by Core Library~~ Add Deprecated APIs Depended on Other Libraries Apr 10, 2025

biniona changed the title ~~Add Deprecated APIs Depended on Other Libraries~~ Add Deprecated APIs Depended on by Legacy Tools Apr 10, 2025

biniona added 17 commits April 10, 2025 15:21

bump version

1f99a48

deprecation warnings

d17127d

add test

d8044a6

make _get_meta explicitly exported

d7fc623

remove unused APIs

121565e

format

cc50286

use master branch version of function

561f88c

use master contents of registry function

688fb1d

use master inject.function

0ef71ba

update tests to reflect things removed from reversing commits

d619d85

add start/close tests

e12457c

remove unused types

c514a8b

add deprecation notice to metadata

3f4be81

make registry wrapper private - not referenced

e659af9

add additional deprecations

b6991c7

format

d3346a3

fix type error

0e4042f

biniona commented Apr 10, 2025

View reviewed changes

biniona marked this pull request as ready for review April 10, 2025 21:50

biniona requested a review from bcmills April 10, 2025 21:50

bcmills reviewed Apr 11, 2025

View reviewed changes

bcmills approved these changes Apr 11, 2025

View reviewed changes

biniona added 5 commits April 11, 2025 13:47

bcm - add all public methods to __all__

b9b7175

bcm - suggestions

e4b15d4

bcm - clamp down start_method close_method api

fe7c9b0

bcm - doc comment

b4e37d6

bcm - port some type fixes

ea0f228

biniona requested a review from mmchenry-duolingo April 11, 2025 19:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Deprecated APIs Depended on by Legacy Tools #70

Add Deprecated APIs Depended on by Legacy Tools #70

biniona commented Apr 10, 2025 •

edited

Loading

biniona Apr 10, 2025

biniona Apr 10, 2025

bcmills Apr 11, 2025

biniona Apr 11, 2025 •

edited

Loading

bcmills Apr 11, 2025

biniona Apr 11, 2025

bcmills Apr 11, 2025

bcmills Apr 11, 2025

bcmills Apr 11, 2025

biniona Apr 11, 2025

bcmills Apr 11, 2025

bcmills Apr 11, 2025

biniona Apr 11, 2025 •

edited

Loading

biniona commented Apr 11, 2025

		return getattr(module, var_name)


		class _RegistryWrapper(Generic[T]):

Add Deprecated APIs Depended on by Legacy Tools #70

Are you sure you want to change the base?

Add Deprecated APIs Depended on by Legacy Tools #70

Conversation

biniona commented Apr 10, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

biniona Apr 11, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

biniona Apr 11, 2025 • edited Loading

Choose a reason for hiding this comment

biniona commented Apr 11, 2025

biniona commented Apr 10, 2025 •

edited

Loading

biniona Apr 11, 2025 •

edited

Loading

biniona Apr 11, 2025 •

edited

Loading