Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Clarify solver package responsibility. #2647

Merged

Conversation

benluddy
Copy link
Contributor

@benluddy benluddy commented Feb 16, 2022

As part of preparing the resolution component (and subcomponents) for
use as a library, this clarifies the responsibility of the solver
package as a general-purpose boolean constraint satisfiability solver.
Renaming the Variable type -- formerly Installable -- avoids
unnecessarily tying it to the resolver package's operator
installability use case. The solver package finds solutions to
constraint satisfaction problems in general.

@openshift-ci
Copy link

openshift-ci bot commented Feb 16, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: benluddy

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 16, 2022
@benluddy
Copy link
Contributor Author

/cc @timflannagan

@@ -107,8 +107,8 @@ func (d *litMapping) ConstraintOf(m z.Lit) AppliedConstraint {
}
d.errs = append(d.errs, fmt.Errorf("no constraint corresponding to %s", m))
return AppliedConstraint{
Installable: zeroInstallable{},
Constraint: zeroConstraint{},
Variable: zeroVariable{},
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

Comment on lines +17 to 18
// Variable values are the basic unit of problems and solutions
// understood by this package.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any value in beefing up this godoc comment to tie it back to the CSP?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a package-level summary that addresses this, PTAL.

@timflannagan
Copy link
Contributor

Thanks - I think this is a step in the right direction, as variables like BundleInstallable didn't make much intuitive sense to me while reading these packages throughout the codebase. The only other thing I'd like to see is better godoc comments any user-facing type/interface/etc. that would make traversing this package a bit easier, and could help us work towards making the responsibilities of each level of abstraction here a bit more clear.

As part of preparing the resolution component (and subcomponents) for
use as a library, this clarifies the responsibility of the solver
package as a general-purpose boolean constraint satisfiability solver.
Renaming the Variable type -- formerly Installable -- avoids
unnecessarily tying it to the resolver package's operator
installability use case. The solver package finds solutions to
constraint satisfaction problems in general.

Signed-off-by: Ben Luddy <bluddy@redhat.com>
@benluddy benluddy changed the title Rename solver.Installable to solver.Variable. Clarify solver package responsibility. Feb 16, 2022
@timflannagan
Copy link
Contributor

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Feb 16, 2022
@openshift-merge-robot openshift-merge-robot merged commit 0666fff into operator-framework:master Feb 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants