Skip to content
This repository was archived by the owner on Sep 15, 2025. It is now read-only.

Fix incorrect error message implementation in WordPressAPIError - #662

Merged
crazytonyli merged 4 commits into
trunkfrom
fix-incorrect-error-message
Dec 14, 2023
Merged

Fix incorrect error message implementation in WordPressAPIError#662
crazytonyli merged 4 commits into
trunkfrom
fix-incorrect-error-message

Conversation

@crazytonyli

Copy link
Copy Markdown
Contributor

Description

I made two mistakes in the existing implementation:

  1. A copy-paste error where I should have made WordPressAPIError conforms to LocalizedError.
  2. The implementation should always return a non-nil error message, but I missed that in the endpointError case, because errorDescription is String?.

Testing Details

I have added unit tests in this PR to verify the fixes.


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

@crazytonyli
crazytonyli requested a review from mokagio December 13, 2023 21:49

@mokagio mokagio left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Would it be worth adding tests for the other errorDescription mappings, or since they are logic free we can do without?

I don't have an answer for that, but the criteria I usually use is "how likely is this to change and how important is it for us that it changes intentionally"

Comment thread WordPressKit/WordPressAPIError.swift Outdated
return Self.unknownErrorMessage
case let .endpointError(error):
return error.errorDescription
return error.errorDescription ?? Self.unknownErrorMessage

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The implementation should always return a non-nil error message

Gotcha. So even though errorDescription at the LocalizedError level allows for a nil value, we agree not to do that.

Would it be worth documenting it?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Maybe... there could be an internal _errorDescription or strongErrorDescription or some other better name, that is String and public var errorDescription: String? could delegate to that. The idea being to have the compiler help us remember not to return any Optional.none.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Good point. I didn't make more changes to the implementation because I thought the returning nil case is covered by the added unit tests. But I'll add a bit documentation too.

Comment thread WordPressKitTests/Utilities/WordPressAPIErrorTests.swift
crazytonyli and others added 2 commits December 14, 2023 19:01
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants