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

Avoid casting to NSError in favor of WordPressApiError #779

Open
wants to merge 4 commits into
base: trunk
Choose a base branch
from

Conversation

mokagio
Copy link
Contributor

@mokagio mokagio commented Apr 5, 2024

See discussion at
#777 (comment) with @crazytonyli

Testing Details

See green CI.


  • Please check here if your pull request includes additional test coverage. — N.A.
  • I have considered if this change warrants release notes and have added them to the appropriate section in the CHANGELOG.md if necessary. — N.A.

let jetpackError = JetpackInstallError(
title: endpointError.localizedDescription,
code: endpointError.errorCode,
key: endpointError.errorUserInfo[WordPressComRestApi.ErrorKeyErrorCode] as? String
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe you can do endpointError.apiErrorCode instead of using errorUserInfo.

return
}

let status = RewindStatus(state: .unavailable)
success(status)
failure(endpointError)
Copy link
Contributor

Choose a reason for hiding this comment

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

The original code returns error. But there it's changed to endpointError. I don't think we should change it here?

// FIXME: A hack to support free WPCom sites and Rewind.
// Should be obsolete as soon as the backend stops returning 412's for those sites.
guard error.castedToEndpointErrorWitCode(.preconditionFailure) != nil else {
success(RewindStatus(state: .unavailable))
return
}
Copy link
Contributor

Choose a reason for hiding this comment

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

My brain can't process these two guard statements. But, are their logic changed?

Copy link
Contributor

Choose a reason for hiding this comment

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

I noticed an unit test failed, I think this guard change here might be the cause?

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