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

[CIS-1273] Ensure optional Core Data attributes map to optional Swift types #1646

Merged
merged 1 commit into from Dec 21, 2021

Conversation

jeroenleenarts
Copy link
Contributor

@jeroenleenarts jeroenleenarts commented Nov 24, 2021

🔗 Issue Link

CIS-1273

🎯 Goal

A number of differences between the Core Data model and the ManagedObjects were present. This pull request removes those issues. When applicable a sensible default is chosen for what Core Data describes as primitive types.

🛠 Implementation

Several model definitions were changed, several managed object classes were changed, several sensible default values were chosen.

🧪 Testing

Run through the app. No crashes should occur.

🎨 Changes

Add relevant screenshots or videos showcasing the changes.

☑️ Checklist

  • I have signed the Stream CLA (required)
  • Changelog is updated with client-facing changes
  • New code is covered by unit tests
  • Affected documentation updated (docusaurus, tutorial, CMS (task created)

@jeroenleenarts jeroenleenarts requested a review from a team as a code owner November 24, 2021 13:31
@jeroenleenarts
Copy link
Contributor Author

@tbarbugli I looked into automatically comparing the model definitions in the core data model against the managed object definitions. I was unsuccessful. The one thing I kept running into is the lack of information on the managed objects at runtime. I have no way to request the nullability of properties defined on te class level.

Getting the same info from Core Data is easy.

@Stream-iOS-Bot
Copy link
Collaborator

Stream-iOS-Bot commented Nov 24, 2021

1 Message
📖 There seems to be app changes but CHANGELOG wasn't modified.
Please include an entry if the PR includes user-facing changes.
You can find it at CHANGELOG.md.

Generated by 🚫 Danger

@codecov
Copy link

codecov bot commented Nov 24, 2021

Codecov Report

Merging #1646 (f22a40d) into develop (e921e8f) will decrease coverage by 0.05%.
The diff coverage is 79.59%.

❗ Current head f22a40d differs from pull request most recent head bb70a8a. Consider uploading reports for the commit bb70a8a to get more accurate results
Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #1646      +/-   ##
===========================================
- Coverage    85.24%   85.18%   -0.06%     
===========================================
  Files          231      230       -1     
  Lines        11071    11066       -5     
===========================================
- Hits          9437     9427      -10     
- Misses        1634     1639       +5     
Flag Coverage Δ
llc-tests 85.18% <79.59%> (-0.06%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...rces/StreamChat/Database/DTOs/ChannelMuteDTO.swift 94.73% <ø> (ø)
...reamChat/Database/DTOs/MessageSearchQueryDTO.swift 0.00% <ø> (ø)
...treamChat/Models/Attachments/AttachmentTypes.swift 86.66% <ø> (ø)
Sources/StreamChat/Models/ChatMessage.swift 93.24% <ø> (ø)
Sources/StreamChat/Database/DTOs/MessageDTO.swift 88.24% <72.00%> (-0.15%) ⬇️
...rces/StreamChat/Database/DTOs/ChannelReadDTO.swift 95.38% <80.00%> (-1.34%) ⬇️
...urces/StreamChat/Database/DTOs/AttachmentDTO.swift 83.01% <87.50%> (-0.75%) ⬇️
.../StreamChat/Database/DTOs/MessageReactionDTO.swift 94.66% <90.00%> (-0.14%) ⬇️
...ventMiddlewares/ChannelReadUpdaterMiddleware.swift 91.30% <100.00%> (ø)
...reamChat/WebSocketClient/Events/EventDecoder.swift 75.00% <0.00%> (-3.58%) ⬇️
... and 7 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e921e8f...bb70a8a. Read the comment docs.

Copy link
Member

@tbarbugli tbarbugli left a comment

Choose a reason for hiding this comment

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

Looks pretty solid, I just left a couple comments

Sources/StreamChat/Database/DTOs/ChannelDTO.swift Outdated Show resolved Hide resolved
Sources/StreamChat/Database/DTOs/MessageDTO.swift Outdated Show resolved Hide resolved
Sources/StreamChat/Database/DTOs/MessageReactionDTO.swift Outdated Show resolved Hide resolved
@jeroenleenarts jeroenleenarts force-pushed the CIS-1273-check-optionals branch 2 times, most recently from bc29d67 to 3de57c3 Compare November 29, 2021 15:41
@bielikb bielikb changed the title Cis 1273 check optionals [CIS-1273] check optionals Dec 3, 2021
@bielikb bielikb changed the title [CIS-1273] check optionals [CIS-1273] Ensure optional Core Data attributes map to optional Swift types Dec 3, 2021
@bielikb bielikb added the 🌐 SDK: StreamChat (LLC) Tasks related to the StreamChat LLC SDK label Dec 3, 2021
@jeroenleenarts jeroenleenarts force-pushed the CIS-1273-check-optionals branch 2 times, most recently from d065943 to 8222d9f Compare December 7, 2021 20:18
@jeroenleenarts jeroenleenarts requested a review from a team December 9, 2021 08:35
@jeroenleenarts jeroenleenarts self-assigned this Dec 9, 2021
bielikb
bielikb previously approved these changes Dec 10, 2021
bielikb
bielikb previously approved these changes Dec 13, 2021
bielikb
bielikb previously approved these changes Dec 15, 2021
@NSManaged private var localProgress: Double
var localState: LocalAttachmentState? {
get {
localStateRaw.flatMap { LocalAttachmentState(rawValue: $0, progress: localProgress) }
LocalAttachmentState(rawValue: localStateRaw, progress: localProgress)
Copy link
Contributor

Choose a reason for hiding this comment

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

What about LocalAttachmentState(rawValue: localStateRaw, progress: localProgress) ?? .unknown so we don't expose an optional?

@polqf polqf changed the base branch from main to develop December 15, 2021 15:47
@jeroenleenarts jeroenleenarts force-pushed the CIS-1273-check-optionals branch 2 times, most recently from 58edc61 to 9237c13 Compare December 17, 2021 14:37
- Uses sensible defaults when possible
- Unit tests are also adjusted
- Populate `ownReactions` and `latestReactions` on MessageDTO when creating a new object, they are non-optinal on the Core Data Model
- Mark `MessageReactionDTO.id` and `MessageDTO.quotedMessage` and `ChannelDto.typeRawValue as non-optional
- Make localStateRaw non optional on both the MessageReactionDTO and the AttachmentDTO.
@bielikb bielikb merged commit def5be4 into develop Dec 21, 2021
@bielikb bielikb deleted the CIS-1273-check-optionals branch December 21, 2021 11:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🌐 SDK: StreamChat (LLC) Tasks related to the StreamChat LLC SDK
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants