Codec Interface to Abstract class, made all paths call parseImpl/writeImpl

[ldintr]

Description:

I didn't like how classes could have a 5 signature parse call a 3 signature overload ignoring some parameters (idr where I saw that, it was outside of obj). I would rather a consistent entry (which I'll use to call throwOnError in the future), and a consistent impl.

Here's a quick look. I may change (or rebase in the future) better javadoc.

Checklist

• Documented (Code comments, README, etc.)
• Tested (unit, integration, etc.)

Comments

I noticed I couldn't make this final because sometimes a class overloads it and other times there's no impl

    protected final int writeImpl(@NonNull T item, @NonNull byte[] output, final int startOffset) {
        final BufferedData bufferedData = BufferedData.wrap(output, startOffset, output.length - startOffset);
        try {
            write(item, bufferedData);
        } catch (IOException e) {
            throw new UncheckedIOException(e);
        }
        return (int) bufferedData.position();
    }

It seems like there's a chance they cane become inconsistent

    protected int writeImpl(@NonNull MessageInProtoPackage data, @NonNull byte[] output, final int startOffset) {
        int offset = startOffset;
        // [1] - bytesField
        offset += ProtoArrayWriterTools.writeBytes(output, offset, MessageInProtoPackageSchema.BYTES_FIELD,  data.bytesField(), true);

        // Write unknown fields if there are any
        for (final UnknownField uf : data.getUnknownFields()) {
            final int tag = (uf.field() << TAG_FIELD_OFFSET) | uf.wireType().ordinal();
            offset += ProtoArrayWriterTools.writeUnsignedVarInt(output, offset, tag);
            offset += uf.bytes().writeTo(output, offset);
        }
        return offset - startOffset;
    }

    protected void writeImpl(@NonNull MessageInProtoPackage data, @NonNull final WritableSequentialData out) throws IOException {
            // [1] - bytesField
        writeBytes(out, MessageInProtoPackageSchema.BYTES_FIELD, data.bytesField(), true);

        // Check if not-empty to avoid creating a lambda if there's nothing to write.
        if (!data.getUnknownFields().isEmpty()) {
            data.getUnknownFields().forEach(uf -> {
                final int tag = (uf.field() << TAG_FIELD_OFFSET) | uf.wireType().ordinal();
                out.writeVarInt(tag, false);
                uf.bytes().writeTo(out);
            });
        }
    }

[github-actions]

JUnit Test Report

   521 files  ±0     521 suites  ±0   30s ⏱️ +4s
 1 537 tests ±0   1 533 ✅ ±0   4 💤 ±0  0 ❌ ±0 
10 755 runs  ±0  10 727 ✅ ±0  28 💤 ±0  0 ❌ ±0 

Results for commit 6ff1bff. ± Comparison against base commit fd1fa54.

This pull request removes 6 and adds 6 tests. Note that renamed tests count towards both.

com.hedera.pbj.runtime.ProtoWriterToolsTest ‑ [1] FLOAT, com.hedera.pbj.runtime.ProtoWriterToolsTest$$Lambda/0x000000005e49cd30@6df4d8f1, [0.1, 0.5, 100.0], 12, com.hedera.pbj.runtime.ProtoWriterToolsTest$$Lambda/0x000000005e49cf58@735b1ad2
com.hedera.pbj.runtime.ProtoWriterToolsTest ‑ [1] STRING, com.hedera.pbj.runtime.ProtoWriterToolsTest$$Lambda/0x000000005e4a91b8@230e163b, [string 1, testing here, testing there], com.hedera.pbj.runtime.ProtoWriterToolsTest$$Lambda/0x000000005e4a93e0@7a41f4a7
com.hedera.pbj.runtime.ProtoWriterToolsTest ‑ [2] BYTES, com.hedera.pbj.runtime.ProtoWriterToolsTest$$Lambda/0x000000005e4a9608@7164e54, [010203, ff7f0f, 42da07370bff], com.hedera.pbj.runtime.ProtoWriterToolsTest$$Lambda/0x000000005e4a9830@20040c6e
com.hedera.pbj.runtime.ProtoWriterToolsTest ‑ [2] DOUBLE, com.hedera.pbj.runtime.ProtoWriterToolsTest$$Lambda/0x000000005e49d180@88fc9cb, [0.1, 0.5, 100.0, 1.7653472635472653E240], 32, com.hedera.pbj.runtime.ProtoWriterToolsTest$$Lambda/0x000000005e49d3a8@75693526
com.hedera.pbj.runtime.ProtoWriterToolsTest ‑ [3] BOOL, com.hedera.pbj.runtime.ProtoWriterToolsTest$$Lambda/0x000000005e49d5d0@603a34e5, [true, false, false, true, true, true], 6, com.hedera.pbj.runtime.ProtoWriterToolsTest$$Lambda/0x000000005e49d7f8@160d68b8
com.hedera.pbj.runtime.ProtoWriterToolsTest ‑ [4] ENUM, com.hedera.pbj.runtime.ProtoWriterToolsTest$$Lambda/0x000000005e49da20@1d77d9c6, [0, 2, 1], 3, com.hedera.pbj.runtime.ProtoWriterToolsTest$$Lambda/0x000000005e49dc48@2c5b4b91

com.hedera.pbj.runtime.ProtoWriterToolsTest ‑ [1] FLOAT, com.hedera.pbj.runtime.ProtoWriterToolsTest$$Lambda/0x000000009449cac0@634d56eb, [0.1, 0.5, 100.0], 12, com.hedera.pbj.runtime.ProtoWriterToolsTest$$Lambda/0x000000009449cce8@4cd7128
com.hedera.pbj.runtime.ProtoWriterToolsTest ‑ [1] STRING, com.hedera.pbj.runtime.ProtoWriterToolsTest$$Lambda/0x00000000944a8f68@476dc0af, [string 1, testing here, testing there], com.hedera.pbj.runtime.ProtoWriterToolsTest$$Lambda/0x00000000944a9190@797ca322
com.hedera.pbj.runtime.ProtoWriterToolsTest ‑ [2] BYTES, com.hedera.pbj.runtime.ProtoWriterToolsTest$$Lambda/0x00000000944a93b8@6a827b0c, [010203, ff7f0f, 42da07370bff], com.hedera.pbj.runtime.ProtoWriterToolsTest$$Lambda/0x00000000944a95e0@4cff38ff
com.hedera.pbj.runtime.ProtoWriterToolsTest ‑ [2] DOUBLE, com.hedera.pbj.runtime.ProtoWriterToolsTest$$Lambda/0x000000009449cf10@7916568d, [0.1, 0.5, 100.0, 1.7653472635472653E240], 32, com.hedera.pbj.runtime.ProtoWriterToolsTest$$Lambda/0x000000009449d138@58fadccb
com.hedera.pbj.runtime.ProtoWriterToolsTest ‑ [3] BOOL, com.hedera.pbj.runtime.ProtoWriterToolsTest$$Lambda/0x000000009449d360@43d2c8e3, [true, false, false, true, true, true], 6, com.hedera.pbj.runtime.ProtoWriterToolsTest$$Lambda/0x000000009449d588@8bb5c2a
com.hedera.pbj.runtime.ProtoWriterToolsTest ‑ [4] ENUM, com.hedera.pbj.runtime.ProtoWriterToolsTest$$Lambda/0x000000009449d7b0@a5df98c, [0, 2, 1], 3, com.hedera.pbj.runtime.ProtoWriterToolsTest$$Lambda/0x000000009449d9d8@5f1dadf4

[github-actions]

Integration Test Report

    428 files  ±0      428 suites  ±0   21m 28s ⏱️ +37s
115 048 tests ±0  115 048 ✅ ±0  0 💤 ±0  0 ❌ ±0 
115 292 runs  ±0  115 292 ✅ ±0  0 💤 ±0  0 ❌ ±0 

Results for commit 6ff1bff. ± Comparison against base commit fd1fa54.

This pull request removes 2 and adds 2 tests. Note that renamed tests count towards both.

com.hedera.pbj.integration.test.ParserNeverWrapsTest ‑ [1] com.hedera.pbj.integration.test.ParserNeverWrapsTest$$Lambda/0x0000000058c30488@6daf1efb
com.hedera.pbj.integration.test.ParserNeverWrapsTest ‑ [2] com.hedera.pbj.integration.test.ParserNeverWrapsTest$$Lambda/0x0000000058c306d0@5821164b

com.hedera.pbj.integration.test.ParserNeverWrapsTest ‑ [1] com.hedera.pbj.integration.test.ParserNeverWrapsTest$$Lambda/0x0000000059be8238@497c922f
com.hedera.pbj.integration.test.ParserNeverWrapsTest ‑ [2] com.hedera.pbj.integration.test.ParserNeverWrapsTest$$Lambda/0x0000000059be8480@176ccc53

[anthony-swirldslabs]

You want to add javadocs to any new protected/public methods. I also left a minor comment. Other than that, the changes look great. Thanks!

[anthony-swirldslabs]

Minor: I'd go as far as to actually nuke all the overloads that supply default argument values. In the current fix where these methods have simply been made final, the benefit of the code simplification is a bit small to my liking.

[anthony-swirldslabs]

How come? Our codec generator here in PBJ is supposed to generate them (or not) unconditionally.

Also, you want to add javadocs to any public/protected methods that you introduce here.