Refactor Translator to use less global state #165
Conversation
In the past, there was one Translator, which was a global singeton, and would be permanently modified any time a new object class with an _item_type was created. Recently, in v2.0.0a1, we tried out a change where the global translator would be modified any time a new object class with an _item_type OR with a baseclass with an _item_type was created. This means that every subclass operation mutated global state. We thought this would make it easier for developers to add their own classes to the SDK. In retrospect, it makes it much harder to write tests (because any temporary subclasses created, including by a mock library, will modify global state for the rest of the test run), and makes it impossible to intentionally create a subclass that shouldn't be registered in the translator. So we are reverting this behavior for v2.0.0a2. Furthermore, this adds the ability to create non-global translators (which are, by default, used on `BoxSession` objects), and the ability to add non-global registrations to these non-global translators. This is now the publicly recommended way for developers to register types, outside of the SDK itself. For now, the old mechanism of implicitly registering the official SDK classes with _item_type is retained. But we can experiment with the new system, and see if we prefer to switch to the explicit registration, and delete the implicit registration system, in v2.0.0a3. Also fix a bug that I discovered in ExtendableEnumMeta.
|
Verified that @jmoldow has signed the CLA. Thanks for the pull request! |
| item_type = attrs.get('_item_type', None) | ||
| if item_type is not None: | ||
| Translator().register(item_type, cls) | ||
| Translator._default_translator.register(item_type, cls) # pylint:disable=protected-access |
There was a problem hiding this comment.
Should we add a method Translator.register_default method instead of accessing this protected attribute?
There was a problem hiding this comment.
When I started this work, I started it with Translator.default_translator. At some point I decided to switch it to Translator._default_translator, to discourage applications from using it.
I could add Translator.register_default, but then that might encourage people to use it.
If we're going to make something public, I think it'd be more useful to make Translator.default_translator public, rather than making it private but adding this public method. Unless you think there's utility in only making the register method public.
I think it's okay for us to access the protected attribute, since it's clearly documented as being for internal use.
Add a `translator` property to `BaseEndpoint`, for use in API call methods. Fix test and lint failures.
| assert isinstance(Translator._default_translator, Translator) # pylint:disable=protected-access | ||
|
|
||
|
|
||
| @pytest.mark.parametrize(('extend_default_translator', 'new_child'), list(product([None, True], [False, True]))) |
There was a problem hiding this comment.
Normally we would make fixture functions here, one for extend_default_translator, and another for new_child.
|
👍 |
In the past, there was one Translator, which was a global
singeton, and would be permanently modified any time a new
object class with an _item_type was created.
Recently, in v2.0.0a1, we tried out a change where the global
translator would be modified any time a new object class with an
_item_type OR with a baseclass with an _item_type was created.
This means that every subclass operation mutated global state.
We thought this would make it easier for developers to add their
own classes to the SDK. In retrospect, it makes it much harder
to write tests (because any temporary subclasses created,
including by a mock library, will modify global state for the
rest of the test run), and makes it impossible to intentionally
create a subclass that shouldn't be registered in the
translator. So we are reverting this behavior for v2.0.0a2.
Furthermore, this adds the ability to create non-global
translators (which are, by default, used on
BoxSessionobjects), and the ability to add non-global registrations to
these non-global translators. This is now the publicly
recommended way for developers to register types, outside of the
SDK itself.
For now, the old mechanism of implicitly registering the
official SDK classes with _item_type is retained. But we can
experiment with the new system, and see if we prefer to switch
to the explicit registration, and delete the implicit
registration system, in v2.0.0a3.
Also fix a bug that I discovered in ExtendableEnumMeta.