Update Label 5Z Slash — R3 HOWGOZIT + enhanced ET branches#421
Update Label 5Z Slash — R3 HOWGOZIT + enhanced ET branches#421thepacket wants to merge 1 commit intoairframesio:masterfrom
Conversation
Extended to handle additional sub-formats and/or bug fixes.
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 55 minutes and 8 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
kevinelliott
left a comment
There was a problem hiding this comment.
Summary
Adds an R3 (Request HOWGOZIT Message) sub-format branch and enriches the existing ET (Expected Time) branch to surface DAY, ESTTAG (e.g. EON), and ESTQUAL (e.g. AUTO) fields instead of dumping them into remaining.text. Also fixes the aiports typo. Lets AUTO-tagged ET estimates reach decodeLevel: 'full'.
Verdict
Comment-only review. The R3 addition is purely additive and the ET enhancement is a clean improvement. One existing test fails because it was written against the old behavior of dumping AUTO into remaining.text.
Must Fix
-
/ET variant 1test fails (Label_5Z_Slash.test.ts:129):- Old assertion:
decodeLevel === 'partial'(becauseremaining.text === 'AUTO'). - New behavior: AUTO is now consumed as
ESTQUAL,remaining.textis empty →decodeLevel === 'full'. - Old assertion:
formatted.items.length === 6— should now be 8 (added DAY row, ESTTAG row, ESTQUAL row). - Old assertion:
remaining.text === 'AUTO'— should be''.
Please update the test to assert
decodeLevel: 'full', the new items count, and the structured ESTTAG/ESTQUAL/DAY rows. Also add a new test for the R3 branch — the documented example/R3 HOWGOZIT REQ / KRSW KIAH 19 213658 2388 19 KRSWwould be ideal. - Old assertion:
Should Fix
- The R3 branch documentation says the trailing fields are
<DAY-echo> <DEP-echo>— but you store them asparts[5]andparts[6]then surface them as a single "Trailing Echo (DAY / DEP)" row. Consider whether echoing them adds value at all; if they always duplicateparts[2]andparts[0], it might be cleaner to drop the row entirely (or at least gate it on a mismatch). - ET branch: new DAY row uses raw code
DAYand labelDay of Month, but the project already hasResultFormatter.day(decodeResult, day)which uses codeMSG_DAYand labelDay of Month. Use the formatter for consistency with other plugins. - Same point for FlightNumber in R3 — you do use
ResultFormatter.flightNumber(), good. The DAY field should also useResultFormatter.day()(or you could call it twice in the existing ET flow). - The ESTTAG and ESTQUAL "(... — wheels-on touchdown)" / "(FMS-generated, not crew-entered)" annotations are great for human readability but slightly mix raw and presentation in
formatted.items[].value. Consumers parsing this string would need to strip the annotation. Consider keeping the parenthesised gloss out ofvalueand exposing it via a separate field, or adding it as a longerdescriptioninstead. Not a blocker, just a stylistic call.
Nits
parts.filter((s) => s !== '')— fine, buts.length > 0reads slightly nicer. Bikeshed.data[2].split(' ').filter(...)— note this assumes a single-space separator. The fixtureKRSW KIAH 19 213658 2388 19 KRSWmatches, but if message formatting drifts to\s+you'll want.split(/\s+/)for robustness.
Backward Compat / Regression Risk
- Existing ET messages with no trailing
AUTO: still parse identically, but now emit a DAY row and (ifeonTagexists) an ESTTAG row even when those values were already there. So consumers will see new rows on every ET message. Reasonable but worth flagging. decodeLevelof ET messages with AUTO upgrades frompartial → full. Strict improvement.- R3 messages were previously falling into the
elsebranch with a debug log only (Decoder: Unkown 5Z RDC format: ...) and the message was decoded aspartial(becausedecodeLevel = decodeResult.remaining.text ? 'partial' : 'full'andremaining.textwould be empty unless something pushed to it — actually since nothing was pushed, R3 was probably falling through to theelsebranch which setsdecoded = false, decodeLevel = 'none'). Either way, R3 is new functionality, so no regression.
Test Coverage
- ET test must be updated.
- Add R3 fixture (
/R3 HOWGOZIT REQ / KRSW KIAH 19 213658 2388 19 KRSW). - Consider an R3 fixture without trailing echo (
parts[5]/parts[6]absent) to verify the conditional doesn't push an empty row.
Outstanding Existing Comments
None. Only an auto-generated CodeRabbit rate-limit notice on the PR.
Thanks @thepacket! Solid additive work — the R3 branch is well-documented and the ET enrichment is exactly the right call. Just needs the existing ET test rewritten and ideally a new R3 test before merge.
Adds the R3 HOWGOZIT sub-format and enhances the ET branch to expose DAY, ESTTAG (EON), ESTQUAL (AUTO) fields instead of dumping into
remaining.text. Lets AUTO-tagged estimates reachlevel: full.npm run buildpasses.