Skip to content

Conversation

@Jenson3210
Copy link
Contributor

As discussed with @sambsnyd and @shanman190, extracting this block Trait again but in a separate PR so we have time to discuss on the correct "contract" of its api.

@github-project-automation github-project-automation bot moved this to In Progress in OpenRewrite Sep 30, 2025
@Jenson3210 Jenson3210 self-assigned this Sep 30, 2025
Comment on lines 54 to 56
public Block filterStatements(Predicate<Statement> predicate, BiFunction<J.Return, Expression, J.Return> returnMapper) {
return mapStatements(statement -> predicate.test(statement) ? statement : null, returnMapper);
}
Copy link
Member

@sambsnyd sambsnyd Sep 30, 2025

Choose a reason for hiding this comment

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

The name filter doesn't quite fit since this also maps.
Feels a bit redundant with mapStatements(). You can already filter by returning null from a mapping.
Since we have the "return null means remove" convention already filter is a subset of map. So for that matter, do we need a separate filterStatements at all or is a single map method sufficient for everything? But the same could be said about ListUtils and we have filter methods there so maybe user convenience justifies the redundancy.

Copy link
Contributor

@shanman190 shanman190 Sep 30, 2025

Choose a reason for hiding this comment

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

So are we trying to be a bit too creative here? What if we just did a single method like withStatements(Function<List<Statement>, List<Statement>> mapper) where we pass in the original list from J.Block, the user can update it however they see fit -- likely using ListUtils * -- then pass us back the updated list which we move or remove comments in relationship to the two lists. This would support all three use cases and put the handling of aspects such as return statements squarely in the author's hands. The with* method here feels fine as well given we're just relaying the state back and forth from the LST, so there's existing precedent for that pattern with other Trait implementations.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: In Progress

Development

Successfully merging this pull request may close these issues.

3 participants