RussellSpitzer commented on PR #17484:
URL: https://github.com/apache/iceberg/pull/17484#issuecomment-5358070197
> Hm, replace is better for perf than replaceAll. It's not called enough to
make huge difference, but I dont see why it's a bad improvement. Moreover, I
think replaceAll doesnt make sense here, as replaceAll interprets regex's
(which is not what is meant in these call sites.)
>
> Maybe we can change the core files? I dont think those particular files
are touched that much? The test changes probably wont matter that much to
warrant church, and we can do it separately if needed
So the general call here is how much do we want to encourage PR's that are
essentially a Noop on the project (not actually more secure or faster.) I'd
feel better if we didn't already have such a huge burden on reviewers but this
is more of a philosophical question. We want to encourage contributors to make
meaningful changes to the code base. Now in this case if we think this is
important and these are the only cases where it happens, rather than a one off
correction we should be locking this out of the codebase entirely.
```xml
<module name="RegexpSinglelineJava">
<property name="id" value="LiteralStringReplaceAll"/>
<property name="ignoreComments" value="true"/>
<property name="format"
value="\.replaceAll\(\s*"([^"\\\\.*+?^$()\[\]{}|]*)""/>
<property name="message" value="Use String.replace for literal
replacements; replaceAll compiles a regex on every call."/>
</module>
```
Again this is only a rule I would add if we already have a tiny surface area
of places to fix since again there really isn't a benefit to doing this but if
we can lock out all future mistakes that feels valid to me.
In case folks aren't aware, what i'm trying to avoid is a situation like
#16881 where we did a rather large PR which was a also essentially a Noop but
it ended up being reverted for the same rational I noted above. That's also
being replicated again in #17534 .
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
