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

refactor: added new types to form the basis of scoring refactor #127

Merged
merged 8 commits into from
Jun 18, 2024

Conversation

j-lanson
Copy link
Collaborator

@j-lanson j-lanson commented Jun 17, 2024

Note: Not sure why the Ubuntu clippy is complaining about unused fields now that it used to not have a problem with. Perhaps a change was made over the past few days that removed Debug from the field usage calculation?

An attempt at defining new types for overhauling the AnalysisResult system and Hipcheck scoring in general. HC prefix was added to structs/variants to disambiguate with existing structs and can/should be renamed once the old system has been removed in a subsequent PR.

The goal of this PR is to produce an AnalysisReport replacement with the following changes:

  • Analysis implementations no longer check db.<X>_active() to decide themselves whether or not to run. The old AnalysisOutcome enum contains the Skip variant which is absent from the new one. Instead, we wrap the HCAnalysisResult in an option to represent the skipped/executed status of an analysis.
  • Analyses no longer determine pass/fail themselves, they will simply return a "value". Thus, Pass, Fail language is also removed from this replacement infrastructure, and analyses won't know or return their own thresholds.
  • An infrastructure that does not require a separate enum variant of AnalysisReport for each analysis type as the existing system does. In this new system, analyses will return a basic/composite value type, and the config/analysis will select from a set of "predicates" that specify how that result is to be interpreted for pass/fail determination. The only one that we have right now is ThresholdPredicate, which allows specifying that a basic value must be above/equal/below a certain value of the same type.

I added the indexmap depedency for the IndexMap object, which is an order-preserving HashMap drop-in replacement. In the past I have found this important for JSON dict comparison. I imagine we will want to update the predicates for comparing composite types.

@j-lanson j-lanson requested a review from alilleybrinker June 17, 2024 15:34
@alilleybrinker
Copy link
Collaborator

There's a new Rust version out as of last Thursday, and I suspect that's the issue with the CI.

@alilleybrinker alilleybrinker added the type: enhancement New feature or request label Jun 17, 2024
@alilleybrinker alilleybrinker added this to the 3.3.0 milestone Jun 17, 2024
hipcheck/src/analysis/result.rs Show resolved Hide resolved
hipcheck/src/analysis/result.rs Outdated Show resolved Hide resolved
hipcheck/src/analysis/result.rs Outdated Show resolved Hide resolved
hipcheck/src/analysis/result.rs Outdated Show resolved Hide resolved
hipcheck/src/analysis/result.rs Show resolved Hide resolved
@alilleybrinker
Copy link
Collaborator

@j-lanson just some minor changes!

@j-lanson j-lanson requested a review from alilleybrinker June 17, 2024 21:01
@alilleybrinker
Copy link
Collaborator

My one remaining comment about turning the &Ordering param to Ordering in pass_threshold, can be done in a follow-up PR.

@alilleybrinker alilleybrinker merged commit 254b30c into main Jun 18, 2024
9 checks passed
@alilleybrinker
Copy link
Collaborator

Thanks @j-lanson!

@alilleybrinker alilleybrinker deleted the jlanson/scoring-breakdown branch June 18, 2024 16:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: enhancement New feature or request
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

2 participants