-
Notifications
You must be signed in to change notification settings - Fork 47
feat: Added support for YunExpress tracking numbers. #109
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
base: main
Are you sure you want to change the base?
Conversation
WalkthroughAdds YunExpress as a supported courier by introducing Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (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 |
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.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
README.md(1 hunks)couriers/yunexpress.json(1 hunks)
🔇 Additional comments (3)
couriers/yunexpress.json (2)
13-22: Test numbers validation looks correct.The test numbers align well with the regex pattern:
- Valid examples have exactly 16 digits (plus YT prefix = 18 total)
- Spaced variant correctly shows optional spaces are handled
- Invalid example with only 15 digits correctly fails the pattern
9-12: YunExpress regex follows existing pattern but conflicts with documented requirement—requires developer decision.The regex indeed lacks a
CheckDigitnamed group, which contradicts the README requirement at line 61. However, this violation is not unique: three other courier files already deployed lackCheckDigit:
amazon.jsonlandmark.jsonlasership.jsonAdditionally, the README's own tracking number table (lines 14–33) lists many couriers with only
SerialNumber, contradicting the stated requirement. No code-level validation was found enforcing this requirement.Decide whether to:
- Remove/relax the requirement from README line 61 (reflecting actual codebase reality)
- Add
CheckDigitgroups to all four files (yunexpress, amazon, landmark, lasership)- Document that
CheckDigitis optional for carriers without checksumsREADME.md (1)
48-48: Formatting change appears reasonable.The blank line after "## JSON Format" separates the section header from the content, improving readability.
| | **UPS** | UPS | 18 | `1Z5R89390357567127` | `SerialNumber` `CheckDigit` `ShipperId` `ServiceType` `PackageId` | | ||
| | | UPS Waybill | 11 | `K2479825491` `V0490119172` | `ServiceType` `SerialNumber` `CheckDigit` | | ||
| | **USPS** | USPS 20 | 20 | `0307 1790 0005 2348 3741` | `ServiceType` `ShipperId` `PackageId` `CheckDigit` | | ||
| | **YunExpress** | YunExpress | 18 | `YT2229521272164446` | `SerialNumber` | |
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.
YunExpress table entry is in the wrong position.
The entry is currently inserted between "UPS Waybill" (line 41) and "USPS 22" (line 43), breaking the alphabetical carrier grouping. YunExpress should be added after all USPS entries (which end at line 45), not in the middle of them.
The carrier order should be: ...UPS Waybill | USPS 20 | USPS 22 | USPS 34v2 | USPS 91 | YunExpress.
Move the YunExpress row to follow the USPS entries. The correct insertion point is after line 45 (USPS 91 entry):
| **USPS** | USPS 91 (IMpb) | 25–34 | `420221539101026837331000039521` `9361289878700317633795` | `RoutingApplicationId` `DestinationZip` `ApplicationIdentifier` `SCNC` `ServiceType` `ShipperId` `PackageId` `CheckDigit` |
+| **YunExpress** | YunExpress | 18 | `YT2229521272164446` | `SerialNumber` |Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In README.md around lines 42 to 45, the YunExpress table row is misplaced
between "UPS Waybill" and "USPS 22"; move the entire YunExpress row (the line
currently at 42) so it appears after the USPS entries (i.e., insert it
immediately after line 45 where the USPS 91 entry ends) to restore correct
alphabetical carrier grouping.
Summary by CodeRabbit
New Features
Documentation