[go: up one dir, main page]

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

Use group/count approach in annual report classes #32914

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

Conversation

mjankowski
Copy link
Contributor

First commit here adds spec coverage which actually exercises the ranking/ordering portion of these classes -- previous coverage asserted correct results/count, but only had one relevant record in returned hash.

Second commit converts some of these reports to rely on the implicit key->count map we get from doing a group by with a count. If the rails-internal count_all naming ever changed here, this query would break but these specs would also break, so I think that's safe to rely on here.

Inspired by #32912 in that it's sort of a similar refactor/approach to making a map of counts.

Related to #31862 in that I think some of those improvements would make sense to resurrect if this merges (specifically, I think the scopes, constants, and private method extractions are useful - even if we dont take all of the more formatting-only type stuff).

Vaguely related to #32807 in that that's also about annual reports.

@mjankowski mjankowski added refactoring Improving code quality ruby Pull requests that update Ruby code labels Nov 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
refactoring Improving code quality ruby Pull requests that update Ruby code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant