Skip to content
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

feat/handle_hex_audiodata #145

Merged
merged 3 commits into from
Oct 18, 2023
Merged

feat/handle_hex_audiodata #145

merged 3 commits into from
Oct 18, 2023

Conversation

JarbasAl
Copy link
Member

@JarbasAl JarbasAl added the enhancement New feature or request label Oct 16, 2023
@JarbasAl JarbasAl requested review from goldyfruit and a team October 16, 2023 23:30
@codecov
Copy link

codecov bot commented Oct 16, 2023

Codecov Report

Attention: 31 lines in your changes are missing coverage. Please review.

Comparison is base (4f65ac5) 60.23% compared to head (a732233) 59.71%.
Report is 5 commits behind head on dev.

Additional details and impacted files
@@            Coverage Diff             @@
##              dev     #145      +/-   ##
==========================================
- Coverage   60.23%   59.71%   -0.52%     
==========================================
  Files          34       34              
  Lines        3347     3366      +19     
==========================================
- Hits         2016     2010       -6     
- Misses       1331     1356      +25     
Files Coverage Δ
ovos_workshop/version.py 0.00% <0.00%> (ø)
ovos_workshop/skills/ovos.py 71.08% <23.07%> (-1.85%) ⬇️

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

# if sessions is not default we also need to do it since
# it likely is a remote client such as hivemind
send_binary = os.environ.get("IS_OVOS_CONTAINER") or \
SessionManager.get(message).session_id != "default"
Copy link
Member

Choose a reason for hiding this comment

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

Is the default session ID guaranteed to always be 'default', or should this reference SessionManager.default_session?

Copy link
Member Author

Choose a reason for hiding this comment

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

it is guaranteed to be "default"

mtype = "mycroft.audio.queue"

if not send_binary:
data = {"uri": filename}
Copy link
Member

Choose a reason for hiding this comment

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

Should this be excluded from binary messages? The URI could be useful for non-default sessions running with a shared file system

Copy link
Member Author

@JarbasAl JarbasAl Oct 17, 2023

Choose a reason for hiding this comment

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

it is not used and further reinforces (to humans debugging) that a different code path is used

i'd rather not send a path that is supposed to be invalid, i'd argue if its running on same device then a non-default session should not be used, but i can imagine a case where a different device still has a shared file system so no strong feelings either way

could also argue this is leaking information about host system (unwanted in some scenarios) and increasing message size/bandwidth usage for no reason

@JarbasAl JarbasAl merged commit f4dca2e into dev Oct 18, 2023
8 of 9 checks passed
@JarbasAl JarbasAl deleted the feat/handle_hex_audiodata branch October 18, 2023 16:27
This was referenced Dec 29, 2023
@github-actions github-actions bot mentioned this pull request Sep 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants