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 reducer to enable testing #85

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

kellyma2
Copy link
Collaborator

@kellyma2 kellyma2 commented Jan 9, 2025

The current iteration of the reducer is a bit difficult to test for a few reasons:

  • it depends on CacheHelper directly
  • the workflow for the reducer is spread around in various command classes
  • loading of repository data and the reduction process are tightly coupled

This PR aims to ameliorate these difficulties by disentangling some of the couple via a separate loader and generic interfaces while also codifying the workflow of the reducer in a single place to make it obvious how it's used

Separating ourselves from the concrete implementation of the
CacheHelper via an interface makes it easier to write tests against
RepoReducer.
The lang option never gets used anywhere (and is not even hooked up as
a proper argument!) so we can safely prune it.
All of the uses of the reducer have the exact same sequence.  We can
capture this sequence and then use it as our model for testing the
reducer.
This is the first step in de-couple the repo data loading from the
reducer itself to enable easier testing of the reducer.
In order to completely decouple the loading process from the general
reducer algorithm we need to decouple it from the RepoReducer itself.

This change converts to returning the package list and accepting the
inputs as parameters to enable this decoupling.
RepoLoader captures the repository package loading process separately
from the reducer and introduces an interface between them to make
testing the reducer easier.
@manuelnaranjo
Copy link
Collaborator

LGTM, I guess tests will be added later. I wonder what that lang variable was for.

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

Successfully merging this pull request may close these issues.

2 participants