-
Notifications
You must be signed in to change notification settings - Fork 6
feat: implement toString() for data classes #4
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR implements custom toString() methods for data classes across the FIDO2 library to improve debugging and logging capabilities. The implementation provides structured, multi-line string representations that include all relevant fields with proper formatting for binary data.
- Adds consistent
toString()implementations usingStringBufferfor formatted output - Includes hex encoding for binary data fields using the
convertpackage - Handles optional fields with conditional formatting
Reviewed Changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 11 comments.
Show a summary per file
| File | Description |
|---|---|
| lib/src/ctap2/requests/make_credential.dart | Adds toString() methods for MakeCredentialRequest and MakeCredentialResponse classes |
| lib/src/ctap2/requests/get_assertion.dart | Adds toString() methods for GetAssertionRequest and GetAssertionResponse classes |
| lib/src/ctap2/requests/credential_mgmt.dart | Adds toString() methods for CredentialManagementRequest and CredentialManagementResponse classes |
| lib/src/ctap2/requests/client_pin.dart | Adds toString() methods for ClientPinRequest and ClientPinResponse classes |
| lib/src/ctap2/pin.dart | Adds toString() method for EncapsulateResult class and updates imports |
| lib/src/ctap2/entities/credential_entities.dart | Updates toString() methods for credential entity classes with improved formatting |
| lib/src/ctap2/entities/authenticator_info.dart | Adds comprehensive toString() method for AuthenticatorInfo class |
| lib/src/ctap2/credmgmt.dart | Adds toString() methods for credential management helper classes |
| lib/src/ctap.dart | Adds toString() methods for CtapResponse and updates CtapError toString() |
| lib/src/cose.dart | Adds toString() methods for COSE key classes with hex-encoded coordinate display |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
lib/src/ctap2/pin.dart
Outdated
| final buffer = StringBuffer(); | ||
| buffer.writeln('EncapsulateResult('); | ||
| buffer.writeln(' coseKey: $coseKey,'); | ||
| buffer.writeln(' sharedSecret: ${hex.encode(sharedSecret)}'); |
Copilot
AI
Aug 16, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing comma after the last field. For consistency with other toString() implementations in this PR, add a comma after 'sharedSecret'.
| buffer.writeln(' sharedSecret: ${hex.encode(sharedSecret)}'); | |
| buffer.writeln(' sharedSecret: ${hex.encode(sharedSecret)},'); |
| return 'PublicKeyCredentialRpEntity(id: $id)'; | ||
| final buffer = StringBuffer(); | ||
| buffer.writeln('PublicKeyCredentialRpEntity('); | ||
| buffer.writeln(' id: $id'); |
Copilot
AI
Aug 16, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing comma after the last field. For consistency with other toString() implementations in this PR, add a comma after 'id'.
| buffer.writeln(' id: $id'); | |
| buffer.writeln(' id: $id,'); |
| buffer.writeln('PublicKeyCredentialUserEntity('); | ||
| buffer.writeln(' id: ${hex.encode(id)},'); | ||
| buffer.writeln(' name: $name,'); | ||
| buffer.writeln(' displayName: $displayName'); |
Copilot
AI
Aug 16, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing comma after the last field. For consistency with other toString() implementations in this PR, add a comma after 'displayName'.
| buffer.writeln(' displayName: $displayName'); | |
| buffer.writeln(' displayName: $displayName,'); |
| buffer.writeln('PublicKeyCredentialDescriptor('); | ||
| buffer.writeln(' type: $type,'); | ||
| buffer.writeln(' id: ${hex.encode(id)},'); | ||
| if (transports != null) buffer.writeln(' transports: $transports,'); |
Copilot
AI
Aug 16, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The 'id' field has a trailing comma but should not have one since it's always the last field before the conditional 'transports' field. Remove the comma or restructure to maintain consistency.
| if (transports != null) buffer.writeln(' transports: $transports,'); | |
| if (transports != null) { | |
| buffer.writeln(' id: ${hex.encode(id)},'); | |
| buffer.writeln(' transports: $transports,'); | |
| } else { | |
| buffer.writeln(' id: ${hex.encode(id)}'); | |
| } |
lib/src/ctap2/credmgmt.dart
Outdated
| buffer.writeln( | ||
| ' existingResidentCredentialsCount: $existingResidentCredentialsCount,'); | ||
| buffer.writeln( | ||
| ' maxPossibleRemainingResidentCredentialsCount: $maxPossibleRemainingResidentCredentialsCount'); |
Copilot
AI
Aug 16, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing comma after the last field. For consistency with other toString() implementations in this PR, add a comma after 'maxPossibleRemainingResidentCredentialsCount'.
| ' maxPossibleRemainingResidentCredentialsCount: $maxPossibleRemainingResidentCredentialsCount'); | |
| ' maxPossibleRemainingResidentCredentialsCount: $maxPossibleRemainingResidentCredentialsCount,'); |
lib/src/ctap.dart
Outdated
| final buffer = StringBuffer(); | ||
| buffer.writeln('CtapError('); | ||
| buffer.writeln(' status: ${status.value},'); | ||
| buffer.writeln(' name: ${status.name}'); |
Copilot
AI
Aug 16, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing comma after the last field. For consistency with other toString() implementations in this PR, add a comma after 'name'.
| buffer.writeln(' name: ${status.name}'); | |
| buffer.writeln(' name: ${status.name},'); |
lib/src/cose.dart
Outdated
| final buffer = StringBuffer(); | ||
| buffer.writeln('CoseKey('); | ||
| buffer.writeln(' algorithm: $alg,'); | ||
| buffer.writeln(' params: ${Map<int, dynamic>.from(this)}'); |
Copilot
AI
Aug 16, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing comma after the last field. For consistency with other toString() implementations in this PR, add a comma after 'params'.
| buffer.writeln(' params: ${Map<int, dynamic>.from(this)}'); | |
| buffer.writeln(' params: ${Map<int, dynamic>.from(this)},'); |
lib/src/cose.dart
Outdated
| final buffer = StringBuffer(); | ||
| buffer.writeln('UnsupportedKey('); | ||
| buffer.writeln(' algorithm: $alg,'); | ||
| buffer.writeln(' params: ${Map<int, dynamic>.from(this)}'); |
Copilot
AI
Aug 16, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing comma after the last field. For consistency with other toString() implementations in this PR, add a comma after 'params'.
| buffer.writeln(' params: ${Map<int, dynamic>.from(this)}'); | |
| buffer.writeln(' params: ${Map<int, dynamic>.from(this)},'); |
lib/src/cose.dart
Outdated
| buffer.writeln('ES256('); | ||
| buffer.writeln(' algorithm: $algorithm,'); | ||
| buffer.writeln(' x: ${x != null ? hex.encode(x) : null},'); | ||
| buffer.writeln(' y: ${y != null ? hex.encode(y) : null}'); |
Copilot
AI
Aug 16, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing comma after the last field. For consistency with other toString() implementations in this PR, add a comma after 'y'.
| buffer.writeln(' y: ${y != null ? hex.encode(y) : null}'); | |
| buffer.writeln(' y: ${y != null ? hex.encode(y) : null},'); |
lib/src/cose.dart
Outdated
| buffer.writeln('EcdhEsHkdf256('); | ||
| buffer.writeln(' algorithm: $algorithm,'); | ||
| buffer.writeln(' x: ${x != null ? hex.encode(x) : null},'); | ||
| buffer.writeln(' y: ${y != null ? hex.encode(y) : null}'); |
Copilot
AI
Aug 16, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing comma after the last field. For consistency with other toString() implementations in this PR, add a comma after 'y'.
| buffer.writeln(' y: ${y != null ? hex.encode(y) : null}'); | |
| buffer.writeln(' y: ${y != null ? hex.encode(y) : null},'); |
|
As we have discussed before, it would be more elegant and clean to use the native serialization of Dart to convert the data classes from/to JSON. @dangfan What's your opinion on this? |
I agree. Shall we finish the server part first and then work on this? |
|
Given our earlier discussion to prefer JSON-based serialization over handcrafted toString, this PR (manual toString) has become obsolete. I’ve implemented the json_serializable-based toJson plus a unified toString via mixin on top of main. Rebasing this branch onto main now causes extensive conflicts due to parallel changes. So I would like to close this PR and open a fresh one from main that reflects the JSON approach for a cleaner review. :D |
No description provided.