Closed cassaundra closed 7 months ago
Side note for those not familiar with internals: this would still catch any nushell compile-time errors since we are still parsing any blocks that are not run
Actually, considering we might want to re-evaluate declaration blocks inside watch mode (e.g. to check new files to add), this seems like an essentially necessary addition.
Implemented as part of #45.
Background
def-task
invocations consist of a declaration block (evaluated eagerly, i.e. inside nushell'sCommand::run
) and/or a run block (evaluated by the build system at a particular point). This approach allows us to easily resolve the build tree inside commands likedepends
, where the dependency ID can be fetched from the global state and inserted into the task in the current context.Proposal
We introduce an alternative run method that reads through the file once to gather "stub" metadata for all tasks, and recursively runs through all declaration blocks starting from the root and continuing through any others as they are depended upon (caching the results into the stub so they are not evaluated more than once).
Performance
This is a pretty low-hanging performance improvement. quake's design philosophy assumes that declaration blocks are relatively cheap to evaluate (at worst, reading a configuration file), but it's not unreasonable to think that relatively expensive operations might occur here (e.g. running
cargo metadata
and transforming the results into a set of subtasks), so from this standpoint this seems worthy enough of a slight design change.Influence on user behavior
Like the subtask transformation example mentioned above, this enables some interesting design patterns. It also allows platform-specific code to be casually run inside declaration blocks if the user knows that the task will only ever be run on a particular platform. However, in order to continue support for the
quake inspect
subcommand (which dumps all task metadata), we'd want to maintain a separaterun_all
method which evaluates all declaration blocks, and in the case of platform-specific code, this might cause significant problems on other systems.For now, we could just strongly recommend inside the documentation to gate those platform-specific commands behind
if $env.os-info
conditionals (also improves readability). Maybe later down the line we could introduce first-class support for this via a requirements system or something similar, but I feel like that goes against quake's minimalist leanings.Implementation details
Since we're evaluating these blocks after the whole file is read, we should probably take the stack into account. Luckily, by this point nushell has resolved variable references (including shadowed variables) into simple
VarId
s, but variable mutations are still problematic. I think the solution least likely to lead to unexpected behavior is just to forbid the capture of variables by making both blocks into closures (which results in no syntactical change, since nushell closures with no arguments can be parsed without any||
, looking just like a block).