Page MenuHomePhabricator

Metaclasses
AbandonedPublic

Authored by max on Mon, Nov 4, 11:01 PM.

Details

Reviewers
alangenfeld
schrockn
Group Reviewers
Restricted Project
Summary

Instead of factory functions, use metaclasses.

Test Plan

Unit

Diff Detail

Repository
R1 dagster
Branch
type-docs
Lint
Lint OK
Unit
No Unit Test Coverage

Event Timeline

max created this revision.Mon, Nov 4, 11:01 PM

why is this better?

themissinghlink added inline comments.
python_modules/dagster/dagster/core/types/field_utils.py
319

A bit confused here (would love some context on what problem you were trying to solve). Metaclasses are a bit funky to read in my opinion and result in some misdirection later on for consumers of it. A factory seems more semantic in this case, but I am willing to be persuaded depending on the problem!

python_modules/dagster/dagster/core/types/field_utils.py
319

As an addendum. Because I hate saying don't use x without giving a good reason. I usually see two common patterns that warrant metaclasses.

A. Implementing a registry pattern to register the creation of instances. This can be easily implemented with decorators IMO.
B. Constructing a sub class. However, the intention is to use new to abstract away connection logic to the parent metaclass. This is why most ORM's use metaclasses so that they can enable the following:

class Person(models.Model):
    first_name = models.String()
    last_name = models.String()

The Model parent is what is instantiating the object and tying everything together.

In this case, I don't really see what advantage __new__ gives you over a factory.

max added a comment.Tue, Nov 5, 1:24 AM

I'm also ok with a move in the opposite direction, where we lowercase selector, permissive_dict, etc., or give them explicit names that make it clear they are factories, This is tricky in docs and the public API because it makes it very clear we are eliding the question, what are they factories of. I think this is a little better because there is a semantic reason we chose to cloak these factories in sentence case to start with, and this respects that while ensuring that the autodocs machinery, etc., will all play nicely with these objects as classes. I view this as a step towards a glorious future where we aren't using these factories to close over environments and return objects that don't pass instanceof checks, but I also don't want to bite off reworking all the .inst() machinery.

schrockn requested changes to this revision.Tue, Nov 5, 3:35 AM

suggest doing something more like this https://dagster.phacility.com/D1358

This revision now requires changes to proceed.Tue, Nov 5, 3:35 AM
max abandoned this revision.Tue, Nov 5, 4:54 PM