feat: Add ToString override to CommandApdu and ResponseApdu #270
Conversation
DennisDyallo
changed the title
There was a problem hiding this comment.
Pull Request Overview
This PR adds human-readable ToString overrides to the APDU classes for improved debugging and logging.
- CommandApdu: prints CLA, INS, P1, P2, Lc (Nc), Le (Ne), and data length.
- ResponseApdu: prints SW1, SW2, and data length.
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| Yubico.Core/src/Yubico/Core/Iso7816/ResponseApdu.cs | Added ToString override to format SW1, SW2, and data length |
| Yubico.Core/src/Yubico/Core/Iso7816/CommandApdu.cs | Added ToString override to format CLA, INS, P1, P2, Lc, Le, and data length |
Comments suppressed due to low confidence (2)
Yubico.Core/src/Yubico/Core/Iso7816/CommandApdu.cs:331
- [nitpick] Printing both
Lc: {Nc}andData: {Data.Length}may be redundant sinceNcshould equalData.Length. Consider printing one or renamingNctoLcto reduce confusion.
public override string ToString() => $"CLA: 0x{Cla:X2} INS: 0x{Ins:X2} P1: 0x{P1:X2} P2: 0x{P2:X2} Lc: {Nc} Le: {Ne} Data: {Data.Length} bytes";
Yubico.Core/src/Yubico/Core/Iso7816/ResponseApdu.cs:89
- There are no existing tests verifying this new
ToStringoutput. Consider adding unit tests to validate the formatting for various APDU responses.
public override string ToString() => $"SW1: 0x{SW1:X2} SW2: 0x{SW2:X2} Data: {Data.Span.Length} bytes";
| @@ -85,5 +85,7 @@ public ResponseApdu(byte[] dataWithoutSW, short sw) | |||
| SW2 = (byte)(sw & 0xFF); | |||
| Data = dataWithoutSW.ToArray(); | |||
| } | |||
|
|
|||
| public override string ToString() => $"SW1: 0x{SW1:X2} SW2: 0x{SW2:X2} Data: {Data.Span.Length} bytes"; | |||
There was a problem hiding this comment.
[nitpick] The use of Data.Span.Length here differs from Data.Length in CommandApdu. Consider unifying the approach (either always use Data.Length on the array or Data.Span.Length) for consistency across both classes.
| public override string ToString() => $"SW1: 0x{SW1:X2} SW2: 0x{SW2:X2} Data: {Data.Span.Length} bytes"; | |
| public override string ToString() => $"SW1: 0x{SW1:X2} SW2: 0x{SW2:X2} Data: {Data.Length} bytes"; |
Copilot uses AI. Check for mistakes.
| @@ -327,5 +327,7 @@ private byte[] GetLeField(ApduEncoding apduEncoding) | |||
|
|
|||
| return leField; | |||
| } | |||
|
|
|||
| public override string ToString() => $"CLA: 0x{Cla:X2} INS: 0x{Ins:X2} P1: 0x{P1:X2} P2: 0x{P2:X2} Lc: {Nc} Le: {Ne} Data: {Data.Length} bytes"; | |||
There was a problem hiding this comment.
[nitpick] The current ToString uses spaces as separators, which can be hard to parse at a glance. Consider adding commas or another delimiter (e.g., CLA: ..., INS: ..., P1: ...) for better readability.
| public override string ToString() => $"CLA: 0x{Cla:X2} INS: 0x{Ins:X2} P1: 0x{P1:X2} P2: 0x{P2:X2} Lc: {Nc} Le: {Ne} Data: {Data.Length} bytes"; | |
| public override string ToString() => | |
| $"CLA: 0x{Cla:X2}, INS: 0x{Ins:X2}, P1: 0x{P1:X2}, P2: 0x{P2:X2}, Lc: {Nc}, Le: {Ne}, Data: {Data.Length} bytes"; |
Copilot uses AI. Check for mistakes.
Test Results: Windows 2 files 2 suites 9s ⏱️ Results for commit d616a08. |
Test Results: Ubuntu 2 files 2 suites 19s ⏱️ Results for commit d616a08. |
Test Results: MacOS 2 files 2 suites 13s ⏱️ Results for commit d616a08. |
Minimum allowed line rate is |
|
@DennisDyallo will this go out with the release tomorrow? If so, I'll add this to the release notes. |
|
@equijano21 No, we'll save this for later :) |
This pull request introduces
ToStringoverrides in theCommandApduandResponseApduclasses to provide a human-readable string representation of their internal state. These changes improve debugging and logging capabilities.Enhancements to debugging and logging:
Yubico.Core/src/Yubico/Core/Iso7816/CommandApdu.cs: Added aToStringoverride to display theCLA,INS,P1,P2,Lc,Le, and the length of theDatafield in a formatted string.Yubico.Core/src/Yubico/Core/Iso7816/ResponseApdu.cs: Added aToStringoverride to display theSW1,SW2, and the length of theDatafield in a formatted string.