mxtymoshyk commented on PR #40008: URL: https://github.com/apache/beam/pull/40008#issuecomment-6002187656
@kennknowles thanks for the review. I took the quick route you suggested: suppress every finding and drop the global filter. All 87 suppressions this PR adds now share one justification: "Pre-existing finding, not triaged yet. Making the class final or moving the throwing code into a static factory method may fix it." The old text said each class could not be made final, and as your comments show, that was wrong for most of them. The 45 classes the PR already made final stay final (82b08fc). I'll do the categorization in follow-up PRs, using the groups from your review: - Classes that aren't public API because of where they live: `runners/`, `examples/`, `it/`, the harness, and the `util`, `fn` and `construction` packages. That's 43 of the 87 and covers most of your inline comments. I'll make them final after checking that nothing subclasses or mocks them. - Refactors to static factory methods, starting with `CompressedSource`, `BeamFnDataOutboundAggregator` and `RowBundle`. - Public classes such as `FixedWindows`, `SlidingWindows` and `CoGbkResult`, where `final` is a judgment call. I'd like your read on those before I change them. - Abstract classes, classes subclassed inside Beam (`SimpleFunction`, `TypeDescriptor`) and classes mocked with Mockito. These keep their suppressions. I'll leave your inline comments open until the follow-up that addresses each one lands. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
