-
Notifications
You must be signed in to change notification settings - Fork 125
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: Store content type parameters #1196
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.
I agree with the general idea, there are just some code changes that can simplify everything I think. Also the classes that currently parse the content-type should be updated (mentioned in one of the comments below).
12642b3
to
24935c2
Compare
I've incorporated all changes discussed before. |
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.
Mostly just some minor nitpicks. Some commits from the main branch also snuck in here.
Co-authored-by: Joachim Van Herwegen <joachimvh@gmail.com>
Co-authored-by: Joachim Van Herwegen <joachimvh@gmail.com>
Co-authored-by: Joachim Van Herwegen <joachimvh@gmail.com>
Co-authored-by: Joachim Van Herwegen <joachimvh@gmail.com>
Co-authored-by: Joachim Van Herwegen <joachimvh@gmail.com>
Co-authored-by: Joachim Van Herwegen <joachimvh@gmail.com>
Co-authored-by: Joachim Van Herwegen <joachimvh@gmail.com>
Co-authored-by: Joachim Van Herwegen <joachimvh@gmail.com>
eee6ef9
to
6a33f61
Compare
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.
Looks good, thanks!
📁 Related issues
#458
✍️ Description
Initial plan, but will not implement now
Replacing the
RepresentationMetadata#contentType
currently in use (as a string) with an object that also contains the parsed parameters would lead to a lot of changes in the codebase (and in testing). The main issue being that the RepresentationMetadata class would no longer consist of only single<identifier> <predicate> <object>
triples to store metadata. The newcontentType
would need something along the lines of this structure:All internal code that now works on the assumption that they can just set a metadata property with one predicate and one value breaks now. The same thing is true for code that removes this triple/quad by identifying it as the triple using the
<ma:format>
predicate.Proposed solution
After some internal discussion with @joachimvh, we've decided on the following strategy for now:
contentType
syntactic sugar on theRepresentationMetadata
class for now so that we don't break any existing code in the CSS.contentTypeObject
to retrieve a newContentType
object with the following structure:contentType
, the optional parameter string is also parsed and stored in the RepresentationMetadata like this:This allows developers who want, to also request the complete
ContentType
object usingmetadata.contentTypeObject
and use the parsed parameters. When assigning a new value tometadata.contentType
, the parameters will first be cleared from the metadata. To explicitly remove all parameters and contentType:metadat.contentType = undefined
does this for you.I've added extra tests to test the behaviour described above.
✅ PR check list
Before this pull request can be merged, a core maintainer will check whether