Add Content-Disposition type
#139
Conversation
adam-fowler
left a comment
adam-fowler
left a comment
There was a problem hiding this comment.
Apart from the one issue this looks good
Welcome to Codecov 🎉Once you merge this PR into your default branch, you're all set! Codecov will compare coverage reports and display results in all future pull requests. Thanks for integrating Codecov - We've got you covered ☂️ |
There was a problem hiding this comment.
Pull Request Overview
This PR introduces a new ContentDisposition type for type-safe parsing of Content-Disposition header fields in multipart messages. The implementation aligns with RFC 6266 and RFC 7578 standards, providing structured access to disposition type, name, filename, and additional parameters.
Key changes:
- Added
ContentDispositionstruct with support for parsing form-data, attachment, and inline disposition types - Replaced direct header parameter manipulation with type-safe parsing
- Added comprehensive test coverage for parsing and error handling
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| Sources/MultipartKit/MultipartPart.swift | Added ContentDisposition type and replaced name property with contentDisposition computed property |
| Sources/MultipartKit/MultipartFormData.swift | Updated to use contentDisposition?.name instead of direct part.name access |
| Sources/MultipartKit/Utilities.swift | Added default value parameter to setParameter method |
| Tests/MultipartKitTests/ContentDispositionTests.swift | Added comprehensive test suite for parsing and error handling |
| Tests/MultipartKitTests/FormDataDecodingTests.swift | Updated test data to use standard disposition types (attachment instead of file) |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| /// - Throws: `ContentDisposition.Error` if the header has an invalid format, or is missing required fields. | ||
| /// - Returns: A parsed `ContentDisposition` instance. | ||
| public var contentDisposition: ContentDisposition? { | ||
| get throws(ContentDisposition.Error) { |
There was a problem hiding this comment.
This claims to throw, but actually returns nil. Either change the behavior or remove both the doc comment and the annotation.
There was a problem hiding this comment.
It returns nil if there's no such header and throws if it's there but unable to parse it (so we also get diagnostics via the error). It makes sense in my head but happy to change the behaviour to just do one of the two
4 participants
Closes #132
This adds a new
ContentDispositiontype to be able to type-safely parse Content-Disposition header fields.Also adds some docs