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

Add a new 'unacceptableStatusCode' error case to WordPressAPIError - #668

Merged
crazytonyli merged 4 commits into
trunkfrom
url-session-helpers-error-on-status-code
Jan 11, 2024
Merged

Add a new 'unacceptableStatusCode' error case to WordPressAPIError#668
crazytonyli merged 4 commits into
trunkfrom
url-session-helpers-error-on-status-code

Conversation

@crazytonyli

Copy link
Copy Markdown
Contributor

Description

This PR is a follow-up to #658 (comment).

At the moment, when an API call returns an failure result (non-200 status code), the existing URLSession helper returns a Swift.Result.success case. After dealing with it a while, I find it too weird... So, this PR changed that by reporting a new WordPressAPIError.unacceptableStatusCode error upon unacceptable status code(non-200 by default, but it is customizable).

This PR introduces breaking changes, in terms of compile time backwards compatibility: swift-case statements on WordPressAPIError may fail. However, I don't think our apps/libraries actually have such statements.

Testing Details

See the updated unit tests.


  • 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.

@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.

Only a non blocking naming suggestion

Comment thread CHANGELOG.md
### Breaking Changes

_None._
- Add a new `unacceptableStatusCode` error case to `WordPressAPIError`. [#668]

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.

How timely. I left a comment on a PR recently that introduced breaking changes, wordpress-mobile/WordPress-iOS#22320 (comment), asking to make them non-breaking with a reminder to update them when we have enough of a corpus of breaking changes to warrant the "effort" of updating WordPressAuthenticator as well.

This seems to be the case now 😄

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.

Followed up here: #678

Comment thread WordPressKit/HTTPClient.swift Outdated
Comment on lines +61 to +62
func mapSuccess<NewSuccess: Decodable, E: LocalizedError>(
_ decoder: JSONDecoder = JSONDecoder()

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.

How do you feel about calling this decodeSuccess?

The motivation for this suggestion comes from looking at the usage in the unit tests:

        let result: WordPressAPIResult<Post, TestError> = await URLSession.shared
            .perform(request: .init(url: URL(string: "https://wordpress.org/hello")!))
            .mapSuccess()

Seeing that mapSuccess call with no argument was surprising as I'm used to seeing map-like calls taking passing a function or a closure.

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.

Renamed in cab530e.

@crazytonyli
crazytonyli enabled auto-merge January 11, 2024 02:46
@crazytonyli
crazytonyli merged commit 92a74b7 into trunk Jan 11, 2024
@crazytonyli
crazytonyli deleted the url-session-helpers-error-on-status-code branch January 11, 2024 02:48
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