docs: add a bunch of docs to dynamic outputs #1047
Conversation
facebook-github-bot
added
the
CLA Signed
|
@facebook-github-bot has imported this pull request. If you are a Meta employee, you can view this in D78294423. (Because this pull request was imported automatically, there will not be any future comments.) |
lf-
force-pushed
the
jade/push-kwvmxrwqmqky
branch
3 times, most recently
from
32daafe to
6b2d388
Compare
| /// [`bxl.dynamic_actions()`](../../bxl/#dynamic_actions) and returns an opaque | ||
| /// [`DynamicValue`](../DynamicValue) thunk containing the result of the call, which can be used |
There was a problem hiding this comment.
The way this is written would seem to imply that the dynamic lambda is evaluated immediately upon invoking actions.dynamic_output_new, but that's obviously not the case - the actual evaluation of that lambda is performed later (if at all). I think we should change this phrasing to clarify that
As a useful piece of context for writing dynamic_output documentation - I've had to explain how this works and why it's useful a number of times internally at Meta at this point and there have consistently been two things that were extremely important to making it "click": 1) Dynamic lambda evaluations are an "analysis," in the sense that they define artifacts and actions, and 2) these evaluations really happen within the action graph, not within the analysis graph
Besides that, I think that starting by talking about DynamicValue in the top-level documentation for dynamic_output is not the best move - DynamicValue is a thing that I would not expect the majority of uses of dynamic_output to require. I think it'd be more appropriate to start by talking about dynamic_outputs ability to bind output artifacts, the fact that it's an analysis, the fact that it can read the contents of other artifacts, and only then mention DynamicValue
| /// Be aware that the context argument of the called impl function differs between | ||
| /// [`dynamic_actions`](../#dynamic_actions) where it is [`actions: AnalysisActions`](../AnalysisActions) | ||
| /// and [`bxl.dynamic_actions`](../../bxl/#dynamic_actions) | ||
| /// where it is [`bxl_ctx: bxl.Context`](../../bxl/Context). |
There was a problem hiding this comment.
I think this belongs on the docs to dynamic_actions/bxl.dynamic_actions moreso than here
| } | ||
|
|
||
| /// Opaque thunk type returned from calling the returned value of a `dynamic_actions` or | ||
| /// `bxl.dynamic_actions` rule invocation. |
There was a problem hiding this comment.
| /// `bxl.dynamic_actions` rule invocation. | |
| /// `bxl.dynamic_actions` invocation. |
This is saying a lot more than I would expect it to though - this thing is semantically just a tuple of the DynamicActionCallable and the params passed into it, all the interesting documentation about those should go onto one of dynamic_action or dynamic_output_new I think
This is useful for the web site. We should probably document how to build the website a little better, but it's mostly okay.
There's a bunch of boilerplate due to facebook#1046. Documents the problem mentioned in facebook#1041, but a more ideal fix would involve the tool itself handling the mixup for you. Fixes: facebook#1038
lf-
force-pushed
the
jade/push-kwvmxrwqmqky
branch
from
6b2d388 to
61104f1
Compare
3 participants
There's a bunch of boilerplate due to
#1046.
Fixes: #1038