-
Notifications
You must be signed in to change notification settings - Fork 24
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
Module for access user-cultivated media, e.g. from a USB drive #119
base: next
Are you sure you want to change the base?
Conversation
{ | ||
"name": "capabilities", | ||
"x-uses": [ | ||
"xrn:firebolt:capability:media-access:*" |
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.
@kpears201 : How should we denote that a method requires one of many possible capabilities?
Currently, if we put more than one capability here, the method requires ALL of them.
I'm really hoping to avoid:
"x-uses": {
"oneOf": [
]
}
as that would fork all of our parsing logic...
I guess we could do something like:
"x-uses": [
],
"x-uses-operator": "or"
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.
Is it necessary to check the capability here?
Consider these two cases:
App has permission to xrn:firebolt:capability:media-access:audio
but does not have permission to xrn:firebolt:capability:media-access:video
(or any other media-access caps)
The app calls MediaAccess.media("all").
What will it return?
Won't it just filter out the videos and not return them and only return the audio.
Now consider removing xrn:firebolt:capability:media-access:audio
permission from the app.
Seems unnecessary that this now returns an error because the app doesn't have permission to anything. Can't we just return an empty list of files because we have filtered out all files it does not have permission to?
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.
that might work, but we have historically tried to avoid api implementations checking specific capabilities, and doing that at the front door. I suppose at some point it is inevitable, though.
"summary": "A set of queries used to match which files will be matched.", | ||
"required": false, | ||
"schema": { | ||
"$ref": "#/components/schemas/MediaFile" |
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.
what does it mean to have both volumes and files fields passed when the Media file also gives the volume
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.
I actually don't understand passing MediaFile at all, should this maybe just be passing a type and not the full MediaFile object?
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.
it's a filter parameter. The volume parameter allows you to filter which volumes you want, using any of the properties of Volume. The MediaFile parameter does the same for files.
{ | ||
"name": "files", | ||
"summary": "A set of queries used to match which files will be matched.", | ||
"required": false, |
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.
you have two optional params for this method, that is not possible with javascript. You will need to wrap this in an object so that it has a single parameter that and these can be passed as optional fields on that object.
I forgot awhile back I was going to propose openrpc detects this and throws a validation error.
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.
it's absolutely possible. It just means you can't leave out the first parameter if you want to use the second one.
I'll take a look at the API and see if we can make the first one required, though.
} | ||
}, | ||
{ | ||
"name": "files", |
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.
the params are called files
and volumes
but the parameter is only a single Volume or MediaFile, I found that confusing.
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.
it's plural because it's a filter, so you're filtering which files you want. maybe I should rename the params to make that more clear.
Core SDK - MFOS standalone sanity report: |
Core SDK - MFOS standalone sanity report: |
Core SDK - MFOS standalone sanity report: |
No description provided.