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

Reduce code duplication in track_hit_feedbacks code #2444

Open
addisoncrump opened this issue Jul 25, 2024 · 3 comments
Open

Reduce code duplication in track_hit_feedbacks code #2444

addisoncrump opened this issue Jul 25, 2024 · 3 comments
Labels
cleanup Reducing our technical debt

Comments

@addisoncrump
Copy link
Collaborator

There's a lot of duplicated Option<bool> code in the feedbacks. This should probably be moved to a wrapper type.

@addisoncrump addisoncrump added enhancement New feature or request cleanup Reducing our technical debt and removed enhancement New feature or request labels Jul 25, 2024
@R9295
Copy link
Collaborator

R9295 commented Jul 29, 2024

Hey Addison,

Since I implemented the feedback hit tracking I have a question regarding this issue:

What would be the benefit of having a wrapper type? This will add another layer of complexity IMO, beneficial only to a very limited subset of Feedbacks.

From my understanding there are 3 different types of feedbacks:

  1. Composite (feedback_and, feedback_or)
  2. Static (feedbacks to annotate metadata for the testcase)
  3. Compute (feedbacks that actually compute data from provided by observers)

Only Computed feedbacks will actually need to cache their results and IMO it makes sense for them to just store last_result in the struct (as they do already). Not sure what static and composite feedbacks have to gain from caching.

I think the duplicated code here is fine since it's just one function.

@addisoncrump
Copy link
Collaborator Author

Makes sense. I think, when I was working on #2438, I was finding lots of duplicated code relating to hit tracking. That is beyond the last_result value in the struct; it's all of the trait changes that the functionality requires.

We can leave it for now, but it feels wrong to slam this into the feedback trait. Maybe we need to revisit how we implement feedbacks s.t. we can somehow add new functionality dynamically.

@addisoncrump addisoncrump changed the title Implement caching feedbacks to deduplicate track_hit_feedbacks code Reduce code duplication in track_hit_feedbacks code Jul 29, 2024
@R9295
Copy link
Collaborator

R9295 commented Jul 29, 2024

But the very nature of the feature implies that every Feedback must implement it? But I agree that Feedbacks need a re-visit, as there are too many static Feedbacks and it feels unintuitive

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cleanup Reducing our technical debt
Projects
None yet
Development

No branches or pull requests

2 participants