Skip to content

feat: apply command in SKEL (SK-167)#227

Open
drmorr0 wants to merge 1 commit intomainfrom
drmorr/skel-apply-cmd
Open

feat: apply command in SKEL (SK-167)#227
drmorr0 wants to merge 1 commit intomainfrom
drmorr/skel-apply-cmd

Conversation

@drmorr0
Copy link
Contributor

@drmorr0 drmorr0 commented Mar 11, 2026

Related Links

  • SK-167

Description and Rationale

  • apply is now a fully-supported SKEL function
  • Checked additional variable errors/issues in the AST generation instead of the engine
  • build module updated to support make debug-test
  • fix the rhs_to_value function in the event that the first component isn't a variable
  • updated the docs

How

Checking defined variables in AST generation

It turns out we were already tracking a defined_vars set in the ast code, so it was a "relatively" simple matter of plumbing that through to more places. It's a little subtle/tricky, because in the left-hand-side of a resource test, you can only reference the variable that's defined in that test, but in the right-hand side you can reference variables that were defined in previous tests. In other words, this is invalid:

$x := some.path | $y == foo

But this is valid:

$x := some.path | exists($x) && $y := some.other.path | $y == $x

We also got rid of the var_prefixed_resource_path rule in the grammar, because it turns out a) it over-complicated the grammar, and b) it disallowed some valid expressions.

Apply SKEL command

This was more-or-less straightforward, we just needed to ensure that all the different variants in the assignment worked correctly (e.g., path = val, $x.path = val, path = $y, and $x.path = $y).

We created a reify_pointers function which takes an abstract pointer and the variable context, and converts that to a list of concrete pointers that match the object with all the variables filled in. This actually simplifies the logic for the remove command as well, and fixes a couple corner cases I didn't think about before (specifically, if the variable substitution doesn't work because we didn't reference a particular variable in the abstract pointer, that would have silently gone through before; I don't think it actually would do anything but still probably isn't ideal).

Test Steps

  • New unit and integration tests written
  • TODO manual testing

Other notes

When I was writing this before, I forgot about the klabel macro, which is super-handy. Would love to get some of this stuff extracted out into a shared library at some point.

  • I certify that this PR does not contain any code that has been generated with GitHub Copilot or any other AI-based code generation tool, in accordance with this project's policies.

@codecov
Copy link

codecov bot commented Mar 11, 2026

Codecov Report

❌ Patch coverage is 97.22222% with 5 lines in your changes missing coverage. Please review.
✅ Project coverage is 78.70%. Comparing base (f7b6f42) to head (dbbeda7).

Files with missing lines Patch % Lines
sk-cli/src/skel/ast.rs 97.56% 2 Missing ⚠️
sk-cli/src/skel/engine.rs 97.46% 2 Missing ⚠️
sk-cli/src/skel/mod.rs 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #227      +/-   ##
==========================================
+ Coverage   78.23%   78.70%   +0.46%     
==========================================
  Files          63       63              
  Lines        3690     3789      +99     
==========================================
+ Hits         2887     2982      +95     
- Misses        803      807       +4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@drmorr0 drmorr0 force-pushed the drmorr/skel-apply-cmd branch 2 times, most recently from dd75036 to d078a3c Compare March 12, 2026 07:03
@drmorr0 drmorr0 changed the title feat: apply command in SKEL feat: apply command in SKEL (SK-167) Mar 12, 2026
@linear
Copy link

linear bot commented Mar 12, 2026

@drmorr0 drmorr0 force-pushed the drmorr/skel-apply-cmd branch 2 times, most recently from 863b0d6 to 3d7fd5d Compare March 12, 2026 23:48
@drmorr0 drmorr0 force-pushed the drmorr/skel-apply-cmd branch from 3d7fd5d to dbbeda7 Compare March 13, 2026 00:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant