spkenv / spk

A Package Manager for high velocity software environments, built on spfs.
https://spkenv.dev
Apache License 2.0
40 stars 6 forks source link

Rethink solver terminology in the code #348

Open jrray opened 2 years ago

jrray commented 2 years ago

I don't want to do this now with all the PRs we have already, but I want to write down my thoughts.

Solver::run does not run a solve. It returns a SolverRuntime.

SolverRuntime does not run a solve, but it holds the state of a solve. Some of the state of the solve is also in Solver.

DecisionFormatter runs a solve.

This is inverted from what would be IMO more natural.

Have Solver take (require) a DecisionFormatter.

Make Solver::run() return Result<Solution> and execute the solve. (Maybe this should be named Solver::runtime()?)

Something about the name SolverRuntime doesn't work for me. In my mind it would become the thing that impl Solve for ... if/when Solve becomes a trait and we have multiple solver implementations. A name like SolverImpl or StepStateSolver which is descriptive of the type of solver it is.

There is also Solver::solve() -> Result<Solution> but nothing should be using this because it doesn't have the behaviors inside DecisionFormatter that everyone expects. If we do the inversion then solve() could use the assigned formatter and run() could go away.

I know the devil is in the details and making these changes might be harder than it seems... DecisionFormatter is closely coupled to the current solver implementation and not generic.

rydrman commented 2 years ago

I agree about the naming and general clarification of responsibilities, and maybe I'll take another opportunity to talk about my current affection for the builder pattern... 😉

I can't help but see the current Solver -> SolverRuntime as more of a SolverBuilder -> Solver relationship. There's some inconsistency right now with which logic/methods are actually attached to which types, but this type of naming would, I think, clarify what belongs where and what each type actually does (setup/initial state vs actually walking through things). This also feels (to me) like it matches more the natural progression where different solver implementations in the future would have different *Builder types. Each one creates a unique type that impls Solver with at least a minimal interface for running it to completion, plus whatever we can standardize in the stepping/progression/reporting stuff (or we just let the builder take a type-specific with_formatter and hide it behind the trait, maybe).