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

Remove unused ReadOnlyReport.append #526

Merged
merged 2 commits into from
Feb 24, 2025
Merged

Conversation

Swatinem
Copy link
Contributor

This method seemed to have been intended to warn about wrong usage of ReadOnlyReport.

I checked both in production logs, as well as scanning through the code, and it looks like we are not misusing the this report class in such a way. So it is safe to remove this warning.

This method seemed to have been intended to warn about wrong usage of `ReadOnlyReport`.

I checked both in production logs, as well as scanning through the code, and it looks like we are not misusing the this report class in such a way. So it is safe to remove this warning.
@Swatinem Swatinem requested a review from a team February 24, 2025 10:21
@Swatinem Swatinem self-assigned this Feb 24, 2025
Copy link

codspeed-hq bot commented Feb 24, 2025

CodSpeed Performance Report

Merging #526 will create unknown performance changes

Comparing swatinem/readonly-append (6320b19) with main (aba4c5c)

Summary

⚠️ No benchmarks were detected in both the base of the PR and the PR.\

Copy link
Contributor

@giovanni-guidini giovanni-guidini left a comment

Choose a reason for hiding this comment

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

Would we benefit from throwing an exception here instead of hoping that we will keep not misusing the ReadOnlyReport?

EDIT ah no I see ReadOnlyReport is not a subclass of any other report, so removing it would cause an exception anyway

@Swatinem Swatinem enabled auto-merge February 24, 2025 13:00
@Swatinem Swatinem disabled auto-merge February 24, 2025 13:00
@Swatinem Swatinem added this pull request to the merge queue Feb 24, 2025
Merged via the queue into main with commit cafa3ea Feb 24, 2025
8 checks passed
@Swatinem Swatinem deleted the swatinem/readonly-append branch February 24, 2025 13:21
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