2703 getter setter #2716
2703 getter setter #2716
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2716 +/- ##
=======================================
Coverage 91.22% 91.23%
=======================================
Files 333 333
Lines 11869 11869
=======================================
+ Hits 10828 10829 +1
+ Misses 1041 1040 -1 ☔ View full report in Codecov by Sentry. |
rorymckinley
force-pushed
the
2703-getter-setter
branch
4 times, most recently
from
388536f to
5141bc7
Compare
rorymckinley
force-pushed
the
2703-getter-setter
branch
from
5141bc7 to
318bc61
Compare
lib/lightning/workflows/snapshot.ex
Outdated
| {:ok, get_current_query(workflow) |> repo.one()} | ||
| end | ||
|
|
||
| multi |> Multi.run(name, get_snapshot) |
There was a problem hiding this comment.
This goes into that single call anti-pattern:
The |> symbol used in the snippet above is the pipe operator: it takes the output from the expression on its left side and passes it as the first argument to the function call on its right side.
Its purpose is to highlight the data being transformed by a *series of functions*.
There was a problem hiding this comment.
@jyeshe changed.
lib/lightning/work_orders.ex
Outdated
| snapshot = Snapshot.get_current_for(attrs[:workflow]) | ||
| changeset |> put_assoc(:snapshot, snapshot) |
There was a problem hiding this comment.
Thanks for the cleanup and I see the piping following the pattern on the if so for existing code or cases like this we could discuss with Stu later.
There was a problem hiding this comment.
@jyeshe Changed.
lib/lightning/workflows/snapshot.ex
Outdated
| @spec include_latest_snapshot(Multi.t(), binary() | :snapshot, Workflow.t()) :: | ||
| Multi.t() | ||
| def include_latest_snapshot(multi, name \\ :snapshot, workflow) do | ||
| get_snapshot = fn repo, _changes -> |
There was a problem hiding this comment.
a named dynamic function is not common on Multi usage but doesn't hurt much. One reason to avoid specially on specific context would be that while reading we split it into two different things while there is only one meaningful operation.
There was a problem hiding this comment.
@jyeshe Changed.
rorymckinley
force-pushed
the
2703-getter-setter
branch
from
318bc61 to
1044631
Compare
rorymckinley
force-pushed
the
2703-getter-setter
branch
from
1044631 to
52e65a2
Compare
taylordowns2000
requested review from
midigofrank
and removed request for
stuartc
Description
This is the first of a series of PRs to cleanup usage of
Snapshot.get_or_create_latest_for. This functionality was widely used to provide coverage as Lightning migrated to snapshots but is now superfluous for many of the use cases as Workflow creation or modification results in Snapshot creation.As some of this PR includes updating test fixtures that were relying on the
or_createfunctionality, I have used distinct commits to make it more digestible.#2703
Validation steps
This functionality was superfluous so there will be no outwardly visible change. The best validation is probably to confirm that my logic as to the removal of the 'or_create' portion of the functionality is correct.
AI Usage
Please disclose how you've used AI in this work (it's cool, we just want to know!):
You can read more details in our Responsible AI Policy
Pre-submission checklist
:owner,:admin,:editor,:viewer)