Conversation
Let's make each report class responsible for its formatting
Now you should only have to update this (and the test) to add something new.
n1zyy
commented
Oct 11, 2023
zachmargolis
approved these changes
Oct 11, 2023
Contributor
zachmargolis
left a comment
There was a problem hiding this comment.
LGTM, got a handful of comments
[skip changelog] ^ because no one outside our team will be interested in this
n1zyy
commented
Oct 12, 2023
| table: account_deletion_report, | ||
| csv_name: 'account_deletion_rate', | ||
| ) | ||
| end |
Contributor
Author
There was a problem hiding this comment.
@ThatSpaceGuy In the event my PR lands before yours, we made some changes after you dropped from pairing, as reflected here.
We moved the Struct out into its own Reporting::EmailableReport "class", and got rid of the metadata stuff. It's all returned inline.
zachmargolis
approved these changes
Oct 12, 2023
Co-authored-by: Zach Margolis <zachmargolis@users.noreply.github.com>
…into mattw/trying-something
olatifflexion
approved these changes
Oct 12, 2023
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
I had an idea and ran with it.
Adding new reports involves some boilerplate duplication—you need to call the report method (unavoidable), remember to upload it to S3, and then add it to the array of arrays. Since I have a story to add a new report, and there are probably about 10 more such stories in the backlog, I was feeling tired of this.
🎫 Ticket
Not requested in any ticket, but rage-coded to lay the groundwork for LG-11148.
🛠 Summary of changes
So, the idea is this:
reportsarray in MonthlyKeyMetricsReport (hereinafter "MKMR").Report.doomarrays defining things like report title and display precision, there is a method added to the report class which contains this information.foo_report, and nowfoo_metadatareturns the metadata. I have added this to all existing reports. There is also afoo_titlemethod which is only used infoo_metadataand need not exist, but it just feels right as a separate method.What I wanted to do was to convention-over-configuration my way out of needing to build as much; in theory knowing there's a
.account_reuse_reportreport method should be enough to automatically call the_metadatamethod as well. But I thought I was already taking on a big change here and that can be a future 🪄 trick.Likewise,
csv_nameshould probably also get pushed into the individualReportingclasses.📜 Testing Plan
This passes automated tests. I am not confident that means it will generate the exact same thing. This should use a little hand-testing, and/or we should update the tests so that if some bozo introduces an "I had an idea and ran with it" PR, we can feel comfortable that the test will catch whether they broke anything.
Right now I mostly want buy-in on whether this is a great refinement or a harebrained mess.