-
Notifications
You must be signed in to change notification settings - Fork 6
Fix missing channel #22
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: master
Are you sure you want to change the base?
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 fixes a bug where the McapError "no channel record found with id X" was being raised incorrectly. The fix moves the channel existence check to occur after filtering logic and allows pre-existing schemas and channels to be passed into the _read_inner
function.
- Moved channel existence validation to occur after message filtering logic
- Added optional
schemas
andchannels
parameters to_read_inner
function - Updated the function call to pass summary schemas and channels when available
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
if isinstance(record, Message): | ||
if record.channel_id not in _channels: | ||
raise McapError(f'no channel record found with id {record.channel_id}') | ||
channel = _channels[record.channel_id] |
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.
This line will raise a KeyError if the channel_id is not in _channels, but the validation check that prevents this error is now placed after this line (at line 140-141). The validation should occur before attempting to access the channel.
Copilot uses AI. Check for mistakes.
src/kappe/reader.py
Outdated
if record.channel_id not in _channels: | ||
raise McapError(f'no channel record found with id {record.channel_id}') |
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.
This validation check is now unreachable because line 133 will already raise a KeyError if the channel_id doesn't exist in _channels. The check should be moved before line 133 to be effective.
Copilot uses AI. Check for mistakes.
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.
Add a test for this? Otherwise LGTM!
This fixes: