jamesfredley commented on PR #15828: URL: https://github.com/apache/grails-core/pull/15828#issuecomment-4997825862
Thanks for the detailed pass (and the Fabled review). These read as review-then-commit nits rather than blockers, and they're all reasonable - I'll take them: 1. **Narrowed element types in `DigestUtils`** - restore the old `Character` / `char[]` tolerance with `DefaultTypeTransformation.castToNumber(value).byteValue()` (keeps the coercion while staying statically compiled). If we decide the narrowing is intentional instead, I'll add a line to the compatibility notes. 2. **`protected` vs `@PackageScope`** - switch to `@PackageScope` so the package-access intent (for `Base64CodecExtensionMethods` / `HexCodecExtensionMethods`) is explicit rather than relying on `protected`'s side effect. 3. **Iterable-target behavior change** - handle `Collection` / `Iterable` in `DigestUtils.toByteArray` so a `Set<Integer>` / `Iterator` hex-encodes its *elements* instead of silently falling through to `toString().getBytes(UTF_8)` (the silent-wrong-output failure mode you flagged). 4. **Hot-loop nits** - `i >> 1` and `str.length() >> 1` instead of `intdiv` (avoids boxing), plain `str.length()`, and drop the redundant `null`/`NullObject` checks (`castToBoolean` already returns false). 5. **MD5/SHA static dispatch** - confirmed intentional (that's the point of removing dynamic dispatch); I'll note in the compatibility section that a runtime metaclass override of `encodeAsMD5Bytes`/SHA variants no longer changes the result. 6. **`HexCodecTests`** - move to the extension-method form (`''.decodeHex()`, `0.decodeHex()`, ...) per the test-via-public-API rule. 7. **`testRoundtrip`** - enable it (add `@Test`) with byte-typed expectations so `assertIterableEquals` compares `Byte` to `Byte`. 8. **`UTF_8`** - make it an added method argument defaulted to `StandardCharsets.UTF_8` in the Base64 methods. I'll push these fixes. On the `GrailsHttpSession` lock conversion - glad that read correctly. Since this is COMMENTED (not blocked), let me know if you'd prefer I split the pure compatibility-note items from the code fixes. -- 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]
