Page MenuHomePhabricator

Make _make_airflow_dag a public api

Authored by catherinewu on Tue, Mar 17, 3:23 AM.


Diff Detail

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

Event Timeline

catherinewu created this revision.Tue, Mar 17, 3:23 AM

hmm, is this what you had in mind?

schrockn requested changes to this revision.Wed, Mar 18, 7:18 PM

Looks good overall.

I would include a test case that has some dummy customer operator and ensures that it runs. You can use that experience to inform the docblock

This revision now requires changes to proceed.Wed, Mar 18, 7:18 PM
Harbormaster completed remote builds in B8835: Diff 10925.
schrockn requested changes to this revision.Tue, Mar 24, 11:31 PM

Lovely. I think we should strongly consider wrapping up all the arguments to init into a single object for terseness. It would just be internal. The public interface (make_airflow_dag_for_operator) would still have all the args exploded.


let's make this a function too. "check_storage_specified" or something? The existing code isn't the best actually (we really shouldn't be doing checks against the environment dict like this anyways). At a minimum let's get this into a function so when we do improve it


Another approach would be for the init method to take a single object that wraps up all of those parameters. Then within the init of *that* object we could do all these checks. Would also reduce the number of lines of code in all of our operator implementations.

This revision now requires changes to proceed.Tue, Mar 24, 11:31 PM

but this is great

schrockn requested changes to this revision.Wed, Mar 25, 3:41 PM

q mgmt re: having a value object instead of N params

This revision now requires changes to proceed.Wed, Mar 25, 3:41 PM


schrockn accepted this revision.Mon, Mar 30, 10:40 PM
schrockn added a subscriber: nate.


fantastic. plz consider that final comment about the listing out props versus the tricksy metaprogramming.

also: cc: @nate who was interested in this




a little cute. i just prefer writing out all the property names here for greppability/understandability etc. even thought it is slightly more code

This revision is now accepted and ready to land.Mon, Mar 30, 10:41 PM
nate added a comment.Tue, Mar 31, 3:13 PM

nice work!

catherinewu marked an inline comment as done.Tue, Mar 31, 9:44 PM
This revision was automatically updated to reflect the committed changes.
schrockn added inline comments.Tue, Mar 31, 10:16 PM

missed this. no reason to make a new class every time. Make this top level