Page MenuHomePhabricator

Configured refactor: IConfigMappable --> ConfiguredMixin
ClosedPublic

Authored by schrockn on Wed, Nov 18, 9:06 PM.

Details

Summary

I was scoping out configured APIs and got frustrated at the
code and documentation duplication. This is my attempt to reconcile
this.

There is definitely some oddness to componesate for the fact
that the node variants (graph and solid) can inherit their
name from the decorated function and other variants cannot. That
causes the passing of original_config_or_config_fn to
copy_for_configured.

However I still like the reduction of surface area. I would like to
reconcile configurable and config mapping into a single capability at
some point, then they both perform the same core task to some degree.

Test Plan

BK

Diff Detail

Repository
R1 dagster
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

Harbormaster returned this revision to the author for changes because remote builds failed.Wed, Nov 18, 9:19 PM
Harbormaster failed remote builds in B21352: Diff 25911!
schrockn retitled this revision from configured refactor to Configured refactor: IConfigMappable --> ConfiguredMixin.
schrockn edited the summary of this revision. (Show Details)

up

going to fail lint

I can't argue with all the big red sections on the left side! I wish phabricator would report on lines deleted vs. added.

Do we want to include copy_for_configured in the public API? I imagine this might be a confusing method for users to encounter.

This revision is now accepted and ready to land.Thu, Nov 19, 12:18 AM