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

Extract Aggregations Class #793

Merged
merged 1 commit into from
Feb 16, 2024
Merged

Extract Aggregations Class #793

merged 1 commit into from
Feb 16, 2024

Conversation

JPrevost
Copy link
Member

@JPrevost JPrevost commented Feb 15, 2024

This is a small change to isolate it from a larger change coming soon where we add a lot more aggregations. Moving Aggregations to its own Class with each Aggregation getting it's own Method with no functional changes is the full scope of this PR.

The next PR will make changes to this new class but the changes will be much easier to follow if we land this small part first.

Developer

  • All new ENV is documented in README
  • All new ENV has been added to Heroku Pipeline, Staging and Prod
  • ANDI or Wave has been run in accordance to
    our guide and
    all issues introduced by these changes have been resolved or opened as new
    issues (link to those issues in the Pull Request details above)
  • Stakeholder approval has been confirmed (or is not needed)

Code Reviewer

  • The commit message is clear and follows our guidelines
    (not just this pull request message)
  • There are appropriate tests covering any new functionality
  • The documentation has been updated or is unnecessary
  • The changes have been verified
  • New dependencies are appropriate or there were no changes

Requires database migrations?

NO

Includes new or updated dependencies?

NO

@mitlib mitlib temporarily deployed to timdex-pr-793 February 15, 2024 21:41 Inactive
@matt-bernhardt matt-bernhardt self-assigned this Feb 16, 2024
Copy link
Member

@matt-bernhardt matt-bernhardt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems straightforward enough; I'm somewhat amused that making this change results in an increase in code coverage (more lines in place means a higher percentage of tested lines of code :-) )

I've checked the playground in the review app and see the same aggregation options, working the same way we expect. :shipit:

@JPrevost JPrevost merged commit 5304eae into main Feb 16, 2024
3 of 4 checks passed
@JPrevost JPrevost deleted the gdt-168-aggregations-part1 branch February 16, 2024 18:31
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.

3 participants