-
Notifications
You must be signed in to change notification settings - Fork 565
Description
Note: still tweaking this, but happy to get any feedback.
Problem
Our current top-level entrypoint, RunScorecard
, has a long list of parameters.
func RunScorecard(ctx context.Context,
repo clients.Repo,
commitSHA string,
commitDepth int,
checksToRun checker.CheckNameToFnMap,
repoClient clients.RepoClient,
ossFuzzRepoClient clients.RepoClient,
ciiClient clients.CIIBestPracticesClient,
vulnsClient clients.VulnerabilitiesClient,
) (ScorecardResult, error)
There’s two problems with this:
- Adding new options would require breaking changes to the function signature. While Scorecard has historically not cared about breaking changes, this is the single most important one, and we’ve broken it countless times when adding features.
- Callers need to provide all parameters, even if they’re fine with the default values. Looking at our own usages, the vast majority of our calls use the default values.
Solution
We can use functional options to solve this problem. Functional options are a paradigm to pass an arbitrary number of options to a function to modify its behavior without changing its signature. At its core is the option type, which is unsurprisingly an alias for a function signature.
type Option func(*somePrivateState) error
The first step is splitting our current arguments into required and optional values. I’m arguing that the only required values are the context and the repo to analyze (for rationale, see Appendix). Every other existing argument will use default values, unless modified by an option:
func Run(ctx context.Context, repo clients.Repo, options ...Option) error {
c := config{
// some sensible defaults
}
for _, option := range options {
if err := option(&c); err != nil {
return err
}
}
...
With this starting point, options are easily added. Let’s use commit SHA and depth as an example, we just add a new field to our unexported state, and an exported option to manipulate it:
type config struct {
commit string
commitDepth int
}
func WithCommitDepth(depth int) Option {
return func(c *config) error {
c.commitDepth = depth
return nil
}
}
func WithCommitSHA(sha string) Option {
return func(c *config) error {
c.commit = sha
return nil
}
}
Existing callers don’t need to change their signature, but those that want to customize the behavior can:
Run(ctx, repo)
Run(ctx, repo, WithCommitDepth(42), WithCommitSHA("foo..."))
Appendix
A lot of the arguments either have default behavior built in already. Or the callers tend to provide the same arguments.
-
commitSHA string
,clients.HeadSHA
is the default, unless someone provides the--commit
option
-
commitDepth int
- defaults to 30 if no option provided link
-
checksToRun checker.CheckNameToFnMap
- The default is to run all checks, but if
--checks
is provided, only those checks are run. However with structured results we're also seeing--probes
added (see 🌱 Add probes to main call #3688)
- The default is to run all checks, but if
-
all of the repo clients
- repoClient clients.RepoClient,
- ossFuzzRepoClient clients.RepoClient,
- ciiClient clients.CIIBestPracticesClient,
- vulnsClient clients.VulnerabilitiesClient,
- internal
- internal usages tend to do one of two things:
- use default clients. e.g.
clients.DefaultCIIBestPracticesClient()
- use
checker.GetClients
, which indirectly creates the default clients
- use default clients. e.g.
- internal usages tend to do one of two things:
- external
Metadata
Metadata
Assignees
Type
Projects
Status