Skip to content

Add WikimateException and centralize API error checks in request() #136

Merged
waldyrious merged 3 commits intohamstar:masterfrom
Xymph:136-central-exception
Aug 31, 2021
Merged

Add WikimateException and centralize API error checks in request() #136
waldyrious merged 3 commits intohamstar:masterfrom
Xymph:136-central-exception

Conversation

@Xymph
Copy link
Copy Markdown
Collaborator

@Xymph Xymph commented Aug 29, 2021

This implements the exception handling portion of #113, where the reasoning for this approach is discussed. Debug logging changes will go into separate PR #139, which will close that issue.

A commit with documentation/changelog edits follows after #135 is wrapped up and merged. (Hmm, I guess this should have been a draft PR. Can it still be changed?)

@waldyrious
Copy link
Copy Markdown
Collaborator

Hmm, I guess this should have been a draft PR. Can it still be changed?

Yep, it can 🙂. You should hopefully see a "Still in progress? Convert to draft" in the PR sidebar (right below the "Reviewers" heading).

@Xymph Xymph marked this pull request as draft August 29, 2021 21:26
@Xymph Xymph force-pushed the 136-central-exception branch from c64897e to 4169410 Compare August 31, 2021 08:13
@Xymph Xymph marked this pull request as ready for review August 31, 2021 09:17
@Xymph Xymph requested a review from waldyrious August 31, 2021 09:18
@Xymph
Copy link
Copy Markdown
Collaborator Author

Xymph commented Aug 31, 2021

One observation about 4169410: request() performs a json_decode but returns the response object, and all callers perform another json_decode, which is a little inefficient. This will be addressed in the next PR that centralizes debug logging too.

Copy link
Copy Markdown
Collaborator

@waldyrious waldyrious left a comment

Choose a reason for hiding this comment

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

LGTM! Just a minor grammar nit 😄

Comment thread Wikimate.php Outdated
@Xymph Xymph force-pushed the 136-central-exception branch from 5250870 to 6d88e77 Compare August 31, 2021 18:07
@waldyrious waldyrious merged commit 59f2bf4 into hamstar:master Aug 31, 2021
@Xymph Xymph deleted the 136-central-exception branch August 31, 2021 18:58
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