Fix vCard bugs; import RFCs #621
Conversation
📝 WalkthroughWalkthroughAdds address-type support to ContactData via a new public nested AddressType enum and constructor overloads; updates vCard/MeCard ADR generation with version-specific address-type mappings and reordered ADR fields; adds an RFC 2426 vCard 3.0 reference file; updates and adds unit tests for address formatting. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Caller
participant ContactData
participant Formatter as VCard/Mecard Formatter
Caller->>ContactData: new ContactData(..., AddressType?)
activate ContactData
ContactData->>ContactData: store fields and _addressType (default HomePreferred if absent)
deactivate ContactData
Caller->>ContactData: ToString(outputType)
activate ContactData
ContactData->>Formatter: select format rules (v2.1 / v3.0 / v4.0 / MECARD)
Formatter-->>ContactData: format spec
ContactData->>ContactData: Map AddressType -> version tokens (GetAddressTypeString21/3/4)
ContactData->>ContactData: compose ADR with reordered fields and type qualifiers
ContactData-->>Caller: return formatted payload string
deactivate ContactData
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (4)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
QRCoder/PayloadGenerator/ContactData_vCard3_rfc2426.txt (1)
1-2356: Consider referencing the RFC instead of including the full text.Including the complete RFC 2426 specification (2,356 lines) significantly increases repository size. Since RFCs are immutable, publicly available documents, consider replacing this file with:
- A brief comment in the code referencing "RFC 2426 - vCard MIME Directory Profile"
- A link in the documentation to https://www.ietf.org/rfc/rfc2426.txt
This approach is more maintainable and is the standard practice in most open-source projects.
QRCoder/PayloadGenerator/ContactData.cs (1)
253-296: Consider throwing an exception for unhandled AddressType values.The three helper methods currently return a default string (
"HOME;PREF","HOME,PREF", or"home,pref") for any unhandledAddressTypevalues. If theAddressTypeenum is extended in the future, this could lead to silent incorrect behavior.Consider either:
- Throwing an
ArgumentOutOfRangeExceptionin the default case to catch unhandled values during development- Explicitly mapping the default case to
AddressType.HomePreferredfor clarityExample:
private string GetAddressTypeString21() { return _addressType switch { AddressType.Home => "HOME", AddressType.Work => "WORK", AddressType.HomePreferred => "HOME;PREF", AddressType.WorkPreferred => "WORK;PREF", - _ => "HOME;PREF" + _ => throw new ArgumentOutOfRangeException(nameof(_addressType), _addressType, "Unknown address type") }; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
QRCoder/PayloadGenerator/ContactData.cs(6 hunks)QRCoder/PayloadGenerator/ContactData_vCard3_rfc2426.txt(1 hunks)QRCoderTests/PayloadGeneratorTests/ContactDataTests.cs(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
QRCoderTests/PayloadGeneratorTests/ContactDataTests.cs (1)
QRCoder/PayloadGenerator/ContactData.cs (5)
PayloadGenerator(5-369)ContactData(10-368)ContactData(58-61)ContactData(87-110)ToString(116-251)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (12)
QRCoderTests/PayloadGeneratorTests/ContactDataTests.cs (6)
50-50: LGTM!The updated expected string correctly reflects the MeCard ADR format with address components in the proper order (PO Box, Extended Address, Street, Locality, Region, Postal Code, Country).
108-108: LGTM!The updated expected string correctly includes the vCard 2.1 address type format (
ADR;HOME;PREF:) and follows RFC 2426 Section 3.2.1 structure for address components.
137-137: LGTM!The updated expected string correctly uses vCard 3.0 TYPE parameter syntax (
ADR;TYPE=HOME,PREF:) with comma-separated type values, consistent with RFC 2426 Section 3.2.1.
166-166: LGTM!The updated expected string correctly uses vCard 4.0 lowercase TYPE parameter syntax (
ADR;TYPE=home,pref:), consistent with vCard 4.0 conventions.
198-223: LGTM!The new test correctly validates work address generation for vCard 3.0. It properly:
- Passes
AddressType.Workexplicitly to the constructor- Expects the correct vCard 3.0 TYPE format (
ADR;TYPE=WORK:)- Handles empty state/region field correctly (double semicolon)
- Follows the established test pattern
225-250: LGTM!The new test correctly validates work preferred address generation for vCard 4.0. It properly:
- Passes
AddressType.WorkPreferredexplicitly to the constructor- Expects the correct vCard 4.0 lowercase TYPE format with PREF (
ADR;TYPE=work,pref:)- Handles empty state/region field correctly (double semicolon)
- Follows the established test pattern
QRCoder/PayloadGenerator/ContactData.cs (6)
32-32: LGTM!The private readonly field
_addressTypefollows C# naming conventions and is consistent with other fields in the class.
58-61: LGTM!The delegating constructor maintains backward compatibility by supplying a default
AddressType.HomePreferred, which matches the previous behavior of the class. This is the standard C# pattern for extending constructors without breaking existing callers.
63-110: LGTM!The new constructor properly extends the existing functionality by:
- Including comprehensive XML documentation for the new
addressTypeparameter- Maintaining all existing parameter assignments
- Adding the
addressTypeassignment at the appropriate location- Following logical parameter ordering (address type after address fields)
142-152: LGTM!The updated comment references RFC 2426 Section 3.2.1, providing helpful context. The MeCard address format correctly omits address type (not supported by MeCard format) and uses comma separators as per MeCard specifications.
217-234: LGTM!The vCard address formatting correctly:
- References RFC 2426 Section 3.2.1 for ADR structure
- Differentiates address type syntax by vCard version (2.1 semicolon-separated, 3.0/4.0 TYPE= comma-separated)
- Uses semicolon field separators per vCard specification
- Follows the RFC 2426 ADR component order: PO Box; Extended Address; Street; Locality; Region; Postal Code; Country
298-322: LGTM!The
AddressTypeenum is well-designed with:
- Clear, comprehensive XML documentation
- Appropriate values covering common use cases (Home, Work, HomePreferred, WorkPreferred)
- Explicit note about
HomePreferredbeing the default for backward compatibility- Proper C# naming conventions
- Logical nesting within the
ContactDataclass
|
While there's lots of improvements that could be done with vCard support, this at least covers the outstanding issues. |
gfoidl
left a comment
gfoidl
left a comment
There was a problem hiding this comment.
this at least covers the outstanding issues.
Please link them in the description here, so they can get auto-closed.
|
They are linked on the right side and will get auto closed: |
Ah, good to know. |
Summary by CodeRabbit
New Features
Documentation
Tests