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

Implement Unwrapped Cells for Improved Type Safety #1382

Open
wants to merge 3 commits into
base: develop
Choose a base branch
from

Conversation

gokulvenkat243
Copy link
Contributor

Issue: #1381

Description:

  • Using Guard let statement they will be prevent the app crashes, when if the cells fails.

@whattherestimefor
Copy link
Contributor

Thank you for noticing this. Please resubmit with the following change: rather than fatalError(), which will crash in production, return nil when the cell is nil or of an unexpected type:

guard let cell = dequeue...(...) as? OurTypeOfCell else {
    assertionFailure("unexpected cell type") // will crash in debug but not in production
    return nil
}

@whattherestimefor whattherestimefor requested review from whattherestimefor and removed request for whattherestimefor January 6, 2025 13:54
@@ -40,12 +40,12 @@ extension ReportSection {
return UITableViewDiffableDataSource(tableView: tableView) { tableView, indexPath, item -> UITableViewCell? in
switch item {
case .header(let headerContext):
let cell = tableView.dequeueReusableCell(withIdentifier: String(describing: ReportHeadlineTableViewCell.self), for: indexPath) as! ReportHeadlineTableViewCell
guard let cell = tableView.dequeueReusableCell(withIdentifier: String(describing: ReportHeadlineTableViewCell.self), for: indexPath) as? ReportHeadlineTableViewCell else { fatalError("WTF?! Wrong cell.") }
Copy link
Contributor

Choose a reason for hiding this comment

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

To avoid crashing in production, please use assertionFailure("unexpected cell dequeued") and then return nil.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

@whattherestimefor whattherestimefor left a comment

Choose a reason for hiding this comment

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

Thank you for making these changes. A couple edits to the spots that aren't quite the same as the others and then this should be good to merge.

@@ -147,12 +166,15 @@ extension StatusSection {
return nil
case .option(let record):
// Fix cell reuse animation issue
let cell: PollOptionTableViewCell = {
guard let cell: PollOptionTableViewCell = {
let _cell = tableView.dequeueReusableCell(withIdentifier: String(describing: PollOptionTableViewCell.self) + "@\(indexPath.row)#\(indexPath.section)") as? PollOptionTableViewCell
_cell?.prepareForReuse()
return _cell ?? PollOptionTableViewCell()
Copy link
Contributor

Choose a reason for hiding this comment

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

This block will never return nil (if _cell is nil, it will create a new PollOptionTableViewCell()), so the else will never be reached. Best fix would be to remove the ?? PollOptionTableViewCell(), so that the block can return nil and the new else can then be reached.

@@ -179,12 +201,15 @@ extension StatusSection {
return nil
case let .history(option):
// Fix cell reuse animation issue
let cell: PollOptionTableViewCell = {
guard let cell: PollOptionTableViewCell = {
let _cell = tableView.dequeueReusableCell(withIdentifier: String(describing: PollOptionTableViewCell.self) + "@\(indexPath.row)#\(indexPath.section)") as? PollOptionTableViewCell
_cell?.prepareForReuse()
return _cell ?? PollOptionTableViewCell()
Copy link
Contributor

Choose a reason for hiding this comment

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

same as above

@@ -218,6 +221,7 @@ extension UITableViewDelegate where Self: DataSourceProvider & MediaPreviewableV
parameters.visiblePath = UIBezierPath(roundedRect: mediaView.bounds, cornerRadius: MediaView.cornerRadius)
return UITargetedPreview(view: mediaView, parameters: parameters)
} else {
assertionFailure("unexpected cell dequeued")
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not a dequeued cell being provided to populate the tableview, but rather a cell that is already in the tableview and is being accessed to learn things about its contents. The assertion could be something like "unexpected cell type at (indexPath)"

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants