You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
There are ordering constraints among some Guidelines. For example, guideline_distance_check should be run last. Also, guideline_code_can_be_downloaded should be checked before guideline_version_has_osi_license or any other guideline that depends on the candidate project's source code being available.
This ordering is currently imposed by the two methods of get_automerge_guidelines: one for NewPackage and one for NewVersion. I have verified that these two methods both present the guidelines they do present in the same order. Having two separate implementations of the ordering offers the risk that those orderings could accidentally differ in the future.
In addition to imposing an ordering, get_automerge_guidelines also tests whether a given Guideline is applicable based implicitly on the new package verses new version distinction, and explicitly on conditions hard coded in those methods rather than encoded as part of the Guideline. These conditions are hard coded in each method -- another opportunutry for assidental inconsistency.
Guidelines have various parameters which control, for example, how strict they should be. These parameters are passed in from the client workflows as keyword arguments to RegistryCI.AutoMerge.run. Currently, each of these must be plumbed through run, pull_request_build, and check! to the Guidelines themselves. This is needlessly cumbersome. It looks like in the past, parameters for new Guidelines have been either passed directly as new arguments to these functions, or included in GitHubAutoMergeData.
Perhaps GitHubAutoMergeData should only contain values passed to and returned from the GitHub API. Isolating the GitHub API interactioon to a single data structure is necessary, but not sufficient to address issues like #438 to use RegistryCI with non-GitHub workflows.
Guideline parameters should be passed cleanly, without modification from run, down through to their respective Guidelines. #492 introduces a Dict for this purpose. Perhaps this should be an immutable struct instead? Then we could be sure it doesn't change, but a new field would need to be added for each Guideline parameter.
Guideline gets a slot whose value is a collection of types for which the Guideline is applicable (NewPackage, NewVersion, or both).
Guideline gets a new slot whose value is a function that determines Guideline applicability.
Any data needed by get_automerge_guidelines should be passed in through GitHubAutoMergeData or the Guideline parameters Dict rather than as ad-hoc arguments.
A single method of get_automerge_guidelines should loop over ALL_GUIDELINES testing which are applicable based on 2 and 3 above.
Move the definition of each guideline to a separate file named by its global variable name, in a guidelines subdirectory. Include guideline files in the order that they should be checked to control the order they appear in ALL_GUIDELINES.
Open Issues
How would we control when update_status should be called? Currently a special token appears in the Guideline orderings returned by get_automerge_guidelines. I don't understand the motivation for this.
Perhaps each Guideline could have a flag that determines if update_status should be called once the Guideline is checked.
The text was updated successfully, but these errors were encountered:
Motivation:
There are ordering constraints among some Guidelines. For example,
guideline_distance_check
should be run last. Also,guideline_code_can_be_downloaded
should be checked beforeguideline_version_has_osi_license
or any other guideline that depends on the candidate project's source code being available.This ordering is currently imposed by the two methods of
get_automerge_guidelines
: one forNewPackage
and one forNewVersion
. I have verified that these two methods both present the guidelines they do present in the same order. Having two separate implementations of the ordering offers the risk that those orderings could accidentally differ in the future.In addition to imposing an ordering,
get_automerge_guidelines
also tests whether a given Guideline is applicable based implicitly on the new package verses new version distinction, and explicitly on conditions hard coded in those methods rather than encoded as part of the Guideline. These conditions are hard coded in each method -- another opportunutry for assidental inconsistency.Guidelines have various parameters which control, for example, how strict they should be. These parameters are passed in from the client workflows as keyword arguments to
RegistryCI.AutoMerge.run
. Currently, each of these must be plumbed throughrun
,pull_request_build
, andcheck!
to the Guidelines themselves. This is needlessly cumbersome. It looks like in the past, parameters for new Guidelines have been either passed directly as new arguments to these functions, or included inGitHubAutoMergeData
.Perhaps
GitHubAutoMergeData
should only contain values passed to and returned from the GitHub API. Isolating the GitHub API interactioon to a single data structure is necessary, but not sufficient to address issues like #438 to use RegistryCI with non-GitHub workflows.Guideline parameters should be passed cleanly, without modification from
run
, down through to their respective Guidelines. #492 introduces a Dict for this purpose. Perhaps this should be an immutable struct instead? Then we could be sure it doesn't change, but a new field would need to be added for each Guideline parameter.Proposal:
The constructor for struct Guideline should append the Guideline to global variable ALL_GUIDELINES. This is implemented in Check that there are "enough" tests and documentation #492.
Guideline gets a slot whose value is a collection of types for which the Guideline is applicable (
NewPackage
,NewVersion
, or both).Guideline gets a new slot whose value is a function that determines Guideline applicability.
Any data needed by
get_automerge_guidelines
should be passed in throughGitHubAutoMergeData
or the Guideline parameters Dict rather than as ad-hoc arguments.A single method of
get_automerge_guidelines
should loop over ALL_GUIDELINES testing which are applicable based on 2 and 3 above.Move the definition of each guideline to a separate file named by its global variable name, in a guidelines subdirectory. Include guideline files in the order that they should be checked to control the order they appear in ALL_GUIDELINES.
Open Issues
How would we control when
update_status
should be called? Currently a special token appears in the Guideline orderings returned byget_automerge_guidelines
. I don't understand the motivation for this.Perhaps each Guideline could have a flag that determines if
update_status
should be called once the Guideline is checked.The text was updated successfully, but these errors were encountered: