Skip to content

Comments

Added default value for file name#262

Open
FSAFTik wants to merge 2 commits intomainfrom
snap_data_fix
Open

Added default value for file name#262
FSAFTik wants to merge 2 commits intomainfrom
snap_data_fix

Conversation

@FSAFTik
Copy link
Contributor

@FSAFTik FSAFTik commented Dec 9, 2025

Purpose

Fixes issue with the empty file name what fails snap sending

Specification

Adds a default value for the file_name attribute in SnapData message

Dependencies & Potential Impact

This fix will allow users not to state file name of the snap

Deployment Plan

None / not applicable

Testing & Validation

None / not applicable

Copilot AI review requested due to automatic review settings December 9, 2025 21:27
@github-actions github-actions bot added the fix Fixing a bug label Dec 9, 2025
klemen1999
klemen1999 previously approved these changes Dec 9, 2025
Copy link
Contributor

Copilot AI left a 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 changes the default value of the file_name parameter in the SnapData class constructor from an empty string to "snap_file", addressing an issue where empty filenames were causing snap sending failures.

Key Changes:

  • Modified the default value of file_name parameter from "" to "snap_file" in SnapData.__init__()

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

snap_name: str,
frame: dai.ImgFrame | dai.EncodedFrame,
file_name: str = "",
file_name: str = "snap_file",
Copy link

Copilot AI Dec 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change breaks the existing test test_snap_data_initialization_minimal in tests/unittests/test_messages/test_snap_data.py at line 28, which asserts that the default file_name is an empty string (""). The test needs to be updated to expect "snap_file" instead to match this new default value.

Copilot uses AI. Check for mistakes.
@klemen1999
Copy link
Collaborator

Needs slight test modifications as mentioned by copilot

@klemen1999 klemen1999 self-requested a review December 9, 2025 21:39
@klemen1999 klemen1999 dismissed their stale review December 9, 2025 21:40

Further changes requested

@klemen1999
Copy link
Collaborator

This PR should also include putting SnapsData and SnapsUploader in the relevant READMEs (here and here respectively)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

fix Fixing a bug

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants