-
Notifications
You must be signed in to change notification settings - Fork 1.4k
Merge mini and verbose reporter implementations #2488
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR! I'm glad to see the reporter tests are still happy.
It's good to combine the reporters into a single class, but there's still a lot of repetition in that class. We need to remove that, otherwise there's little point.
I've left some other comments below.
Co-authored-by: Mark Wubben <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the updates @Michael55555.
I think there's another round of improvements to make. Mainly, I'd like to get this to a place where there's as few surprises as possible for when we next rewrite the reporters. (Combining the two files helps a lot already, of course!)
Something came up and I haven’t yet had a chance to look at this again. |
@Michael55555 I've made a bunch more tweaks. I think it's good to go now. Besides me having already closed it, I don't think this would have solved #653? |
@novemberborn Sounds good! Yes, I just revisited the issue, it's indeed not related to this PR. I can't remember why I referenced it.🤔 |
Great, thank you @Michael55555! |
Happy to help @novemberborn |
This pull request fixes #2217.
IssueHunt Summary
Referenced issues
This pull request has been submitted to: