Adding Label 1J and Label 2J decoding for FTX #231
Conversation
makrsmark
left a comment
makrsmark
left a comment
There was a problem hiding this comment.
Label_H1_PWI was never hooked up and covered by Label_H1
| name = 'label-1j-2j-ftx'; | ||
| qualifiers() { // eslint-disable-line class-methods-use-this | ||
| return { | ||
| labels: ['1J', '2J'], |
There was a problem hiding this comment.
should i split it to each label?
|
Caution Review failedThe pull request is closed. WalkthroughThis update adds a new plugin Changes
Sequence Diagram(s)sequenceDiagram
participant MD as MessageDecoder
participant P as Label_1J_2J_FTX Plugin
participant H1 as H1Helper
MD->>P: Register plugin in constructor
MD->>P: Invoke decode(message)
P->>H1: Call decodeH1Message(message)
H1-->>P: Return decoding result
P-->>MD: Return DecodeResult
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (3)
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![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 (4)
lib/plugins/Label_1J_2J_FTX.ts (1)
14-33: Enhance error handling and logging.The decode method could benefit from more specific error handling:
- Add validation for message format before decoding
- Include label-specific error messages
- Log the specific label being processed when debugging
decode(message: Message, options: Options = {}) : DecodeResult { let decodeResult = this.defaultResult(); decodeResult.decoder.name = this.name; decodeResult.message = message; + const label = message.label || 'unknown'; const msg = message.text.replace(/\n|\r/g, ""); + if (!msg.startsWith('FTX/')) { + if (options.debug) { + console.log(`Decoder: Invalid format for label ${label}: ${message.text}`); + } + ResultFormatter.unknown(decodeResult, message.text); + decodeResult.decoded = false; + decodeResult.decoder.decodeLevel = 'none'; + return decodeResult; + } + const decoded = H1Helper.decodeH1Message(decodeResult, msg); decodeResult.decoded = decoded; decodeResult.decoder.decodeLevel = !decodeResult.remaining.text ? 'full' : 'partial'; if (decodeResult.formatted.items.length === 0) { if (options.debug) { - console.log(`Decoder: Unknown 1J/2J message: ${message.text}`); + console.log(`Decoder: Unknown ${label} message: ${message.text}`); } ResultFormatter.unknown(decodeResult, message.text); decodeResult.decoded = false; decodeResult.decoder.decodeLevel = 'none'; } return decodeResult; }lib/plugins/Label_1J_2J_FTX.test.ts (1)
4-66: Enhance test coverage with additional test cases.While the current tests cover basic functionality, consider adding:
- Edge cases:
- Empty message
- Missing mission number
- Malformed FTX format
- Additional invalid input scenarios:
- Invalid checksum
- Missing required fields
test('handles empty message', () => { const text = ''; const decodeResult = plugin.decode({ text: text }); expect(decodeResult.decoded).toBe(false); expect(decodeResult.decoder.decodeLevel).toBe('none'); }); test('handles missing mission number', () => { const text = 'FTX/ID50007B,RCH4086/MR6,/FX4 QTR PHILLY UP'; const decodeResult = plugin.decode({ text: text }); expect(decodeResult.decoded).toBe(false); expect(decodeResult.decoder.decodeLevel).toBe('none'); });lib/utils/h1_helper.ts (1)
215-220: Add documentation for the 13-character message format.The new condition handles a specific message format of exactly 13 characters, but it's not clear what this format represents or when it's used. Consider adding a comment explaining the structure (first 4 chars, flight number from 4-10, message type from 10-13).
} else if(parts[0].length === 13) { + // Format: XXXX|FFFFFF|YYY where: + // - XXXX: Unknown prefix (stored as unknown) + // - FFFFFF: Flight number + // - YYY: Message type if(processMessageType(decodeResult, parts[0].substring(10))) { ResultFormatter.unknown(decodeResult, parts[0].substring(0, 4)); ResultFormatter.flightNumber(decodeResult, parts[0].slice(4, 10)); return true; }lib/plugins/Label_H1_PWI.test.ts (1)
4-5: Consider updating the test suite description.The test suite description still includes "PWI" despite the refactoring to
Label_H1. Consider updating it to better reflect the purpose of these tests, such as'Label_H1 Weather Report'or simply'Label_H1'.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
lib/MessageDecoder.ts(1 hunks)lib/plugins/Label_1J_2J_FTX.test.ts(1 hunks)lib/plugins/Label_1J_2J_FTX.ts(1 hunks)lib/plugins/Label_H1_FTX.test.ts(1 hunks)lib/plugins/Label_H1_PWI.test.ts(4 hunks)lib/plugins/Label_H1_PWI.ts(0 hunks)lib/plugins/official.ts(1 hunks)lib/utils/h1_helper.ts(1 hunks)
💤 Files with no reviewable changes (1)
- lib/plugins/Label_H1_PWI.ts
✅ Files skipped from review due to trivial changes (1)
- lib/plugins/Label_H1_FTX.test.ts
🔇 Additional comments (12)
lib/plugins/Label_1J_2J_FTX.ts (1)
6-12: Consider splitting label handling for better maintainability.The current implementation handles both '1J' and '2J' labels using the same decoder. Consider splitting them into separate methods or plugins for better maintainability and to handle label-specific parsing requirements.
lib/plugins/official.ts (1)
11-11: LGTM!The export is correctly placed in alphabetical order.
lib/MessageDecoder.ts (1)
27-27: LGTM!The plugin registration is correctly placed in alphabetical order.
lib/utils/h1_helper.ts (3)
212-214: LGTM! Improved readability of position message check.The change from checking the first three characters and length to using
startsWithis more readable and maintainable.
222-222: LGTM! Simplified message type extraction.The change to directly use
substring(0,3)instead of a separate variable makes the code more concise.
209-240:Details
✅ Verification successful
Verify the impact of message type parsing changes.
The changes to the message type parsing logic could affect how different message formats are handled. Let's verify that all supported message formats are still handled correctly.
🏁 Script executed:
#!/bin/bash # Description: Search for test cases that verify different message formats # to ensure they're still handled correctly after the parsing changes. # Look for test files related to H1 message parsing fd -e test.ts -e spec.ts | xargs rg -l "parseMessageType|H1Helper" # Look for example messages in the test files rg "('|\")(?:POS|FPN|FTX|INI|PRG|PWI).*('|\")" -A 5Length of output: 53565
Message Parsing Logic Verified
The test suite covering various supported message formats (POS, FPN, FTX, INI, PRG, and PWI) confirms that the current changes in the message type parsing logic work as expected. Specifically:
- Both one-part and two-part messages are correctly processed.
- The handling of messages starting with "POS" (including the variant with a 13-character string) is verified.
- All related tests across multiple files (e.g. Label_H1_POS, Label_H1_INI, Label_H1_PRG, Label_H1_FPN, Label_H1_PWI) show that the modifications do not introduce regressions.
No issues were found, so the changes appear to have a positive impact on how message types are parsed.
lib/plugins/Label_H1_PWI.test.ts (6)
9-9: LGTM!The plugin initialization has been correctly updated to use the new
Label_H1class.
14-14: LGTM!The plugin name assertion has been correctly updated to match the new
Label_H1class.
27-27: Verify the decode level change.The decode level has been changed from 'full' to 'partial'. Please confirm that this change is intentional and that the decoder is correctly handling the remaining text that it cannot decode.
39-39: LGTM!The new assertion verifies that undecodable text is correctly preserved in the
remaining.textproperty, which is a good practice for partial decoding.
49-49: LGTM!The decoder name assertion has been correctly updated to match the new
Label_H1class.
2-2:Details
❓ Verification inconclusive
Verify the import path for Label_H1.
The import statement has been updated to reflect the refactoring from
Label_H1_PWItoLabel_H1. Let's verify that the file exists at the specified path.
🏁 Script executed:
#!/bin/bash # Description: Verify that Label_H1.ts exists and contains the Label_H1 class. # Test: Check if Label_H1.ts exists fd -t f "Label_H1.ts" lib/plugins # Test: Verify Label_H1 class definition ast-grep --pattern $'class Label_H1 { $$$ }'Length of output: 105
Verify the Label_H1 import and class declaration
The import in
lib/plugins/Label_H1_PWI.test.tshas been updated to import{ Label_H1 }from"./Label_H1", which reflects the refactoring fromLabel_H1_PWItoLabel_H1. We confirmed that the file exists atlib/plugins/Label_H1.tsusing the shell script. However, while the AST grep was executed to verify the presence of aLabel_H1class definition, its output was inconclusive.Please ensure that the
Label_H1class is correctly declared inlib/plugins/Label_H1.ts(e.g., it should be defined asclass Label_H1 { ... }). A manual review or an alternative text search (for example, usingrg "class Label_H1" lib/plugins/Label_H1.ts) might be needed to confirm the class declaration matches the intended structure.
1 participant
Summary by CodeRabbit