Page MenuHomeElementl

Refactor the validation step for positional args in decorators
ClosedPublic

Authored by cdecarolis on Apr 29 2021, 9:24 PM.

Details

Summary

The way we validate required positional arguments in our decorators is hard to understand. This attempts to make the flow a bit easier to follow (and easier to change).

Test Plan

Unit tests

Diff Detail

Repository
R1 dagster
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

Harbormaster returned this revision to the author for changes because remote builds failed.Apr 29 2021, 9:41 PM
Harbormaster failed remote builds in B29724: Diff 36510!

Slight implementation change in preparation for next change

Harbormaster returned this revision to the author for changes because remote builds failed.Apr 29 2021, 10:01 PM
Harbormaster failed remote builds in B29726: Diff 36512!
python_modules/dagster/dagster/core/decorator_utils.py
22

lambda is funky but our error messages here arent uniform

python_modules/dagster/dagster/core/definitions/decorators/solid.py
365

should we get rid of this comment? I dont feel like it makes much sense

365–367

This also is misleading because they're still positional parameters, they're just not the context

some ideas inline - I think this can use a second pass. Its a little better but not a clear step up imo

python_modules/dagster/dagster/core/definitions/decorators/hook.py
48–49

its odd how many of these call sites don't use the output on this function

also agree the lambda for error message is an unfortunate indirection. Would make it hard to improve the error message with more granular context

what if we had a couple more granular functions instead?

get_function_params
has_expected_positionals

split part probably doesnt need a func

python_modules/dagster/dagster/core/definitions/decorators/solid.py
365

ye

365–367

i think its communicating "enforced position" or something maybe but I agree it can be more clear - "input args after the context" or something

372

why this cast? shouldn't the func def get this?

python_modules/dagster/dagster/core/definitions/decorators/solid.py
372

bc is Noneable if a missing positional is returned

Respin with separate fxns for validation and getting extra args

Respin with clearer functionality

Respinning to be more clear about functionality

Respin to have clearer functionality

This revision is now accepted and ready to land.May 3 2021, 4:22 PM