- 
                Notifications
    You must be signed in to change notification settings 
- Fork 0
Separate attachments into a separate library #25
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
Conversation
| 
 Generated by 🚫 Danger | 
| import Foundation | ||
|  | ||
| public final class APIError: @unchecked Sendable, Codable, JSONEncodable, Hashable, ReflectiveStringConvertible { | ||
| public final class APIError: @unchecked Sendable, Codable, Hashable, ReflectiveStringConvertible { | 
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.
JSONEncodable is in StreamOpenAPI at the moment. This should be refactored later, for now, I just removed this protocol conformance. APIError seems OpenAPI related but also gets imported into ClientError.
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 think this is going to be a problem for the video SDK. The api error should be json encodable.
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.
Added back
…ming clash with chat" This reverts commit 9fd4194.
2699bc2    to
    2bfdd08      
    Compare
  
    2bfdd08    to
    ae49c8d      
    Compare
  
    | import Foundation | ||
|  | ||
| public final class APIError: @unchecked Sendable, Codable, JSONEncodable, Hashable, ReflectiveStringConvertible { | ||
| public final class APIError: @unchecked Sendable, Codable, Hashable, ReflectiveStringConvertible { | 
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 think this is going to be a problem for the video SDK. The api error should be json encodable.
| @testable import StreamAttachments | ||
| import Testing | ||
|  | ||
| struct StreamAttachmentsTests { | 
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 believe we should have some tests to move here?
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.
Makes sense to do in a separate PR for reducing the scope of this one. Probably not going to be straight-forward thanks to all the test infrastructure on the chat side.
        
          
                Package.swift
              
                Outdated
          
        
      | targets: ["StreamAttachments"] | ||
| ), | ||
| .library( | ||
| name: "StreamOpenAPI", | 
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's the reasoning of splitting up open API stuff? Eventually chat will use it as well.
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'll think about this one. If I rename new Filter etc types then it would make things simpler. Otherwise there are naming conflicts (chat has Filter which is CoreData based).
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.
Merged them back to a single library
| 
 | 


🔗 Issue Links
Related: IOS-991
🎯 Goal
Separate attachments from core for easier integration in chat
📝 Summary
Provide bullet points with the most important changes in the codebase.
🛠 Implementation
Provide a detailed description of the implementation and explain your decisions if you find them relevant.
🎨 Showcase
Add relevant screenshots and/or videos/gifs to easily see what this PR changes, if applicable.
🧪 Manual Testing Notes
Explain how this change can be tested manually, if applicable.
☑️ Contributor Checklist
docs-contentrepo