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.

Sign up for GitHub

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

Jump to bottom

Improve ClientError handling #209

Merged
merged 17 commits into from
Sep 2, 2024
Merged

Improve ClientError handling #209

merged 17 commits into from
Sep 2, 2024

Conversation

ValentinBuira
Copy link
Contributor

This PR improve the ClientError exception to make it easier to parse and return feedback to user. Instead of relying on a text based error we define variable member of the ClientError Exception than will hold the data response from the server.

Used only for the qgis-plugin for now

@ValentinBuira ValentinBuira mentioned this pull request Jul 9, 2024
Merged
@ValentinBuira
Adress PR comment: use enum over simple string
7883e8a
@ValentinBuira
Sometimes the server responde with a content-type of application/json…
9c7fd95 
… or application/problem+json

e.g :
-create project conflit : Error 409 with content-type application/problem+json
-projects limit hit : Error 422 with content-type application/json

We need to handle both case as they return slightly different structure
@ValentinBuira
clean up : avoid nesting if to generate string representation
34fd1ad
@coveralls
Copy link

coveralls commented Jul 10, 2024 •
edited
Loading

Pull Request Test Coverage Report for Build 10670320966

Details

  • 57 of 70 (81.43%) changed or added relevant lines in 4 files are covered.
  • 1 unchanged line in 1 file lost coverage.
  • Overall coverage increased (+0.02%) to 78.996%
Changes Missing Coverage Covered Lines Changed/Added Lines %
mergin/client.py 11 14 78.57%
mergin/utils.py 1 11 9.09%
Files with Coverage Reduction New Missed Lines %
mergin/client.py 1 83.42%
Totals Coverage Status
Change from base Build 9545211905: 0.02%
Covered Lines: 3193
Relevant Lines: 4042
💛 - Coveralls

wonder-sk
wonder-sk reviewed Jul 22, 2024
View reviewed changes
Copy link
Contributor

@wonder-sk wonder-sk left a comment

Choose a reason for hiding this comment

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

It would be good to add also some auto tests that check for some common errors, making sure ClientError is populated as expected...

mergin/client.py Outdated Show resolved Hide resolved
mergin/client.py Outdated Show resolved Hide resolved
@ValentinBuira
Copy link
Contributor Author

I updated the PR with the requested changes, and it's ready for another review, to sum up:

  • Created two new tests test_error_push_already_named_project and test_error_projects_limit_hit
  • Added a utility function bytes_to_human_size (used in the plugin)
  • Always parse json response from the server the same way whatever content-type is the response

Thanks for your patience

@harminius harminius mentioned this pull request Aug 13, 2024
Merged
MarcelGeo
MarcelGeo requested changes Aug 27, 2024
View reviewed changes
Copy link
Contributor

@MarcelGeo MarcelGeo left a comment

Choose a reason for hiding this comment

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

Just enhancements about tests

mergin/test/test_client.py Outdated Show resolved Hide resolved
mergin/test/test_client.py Outdated Show resolved Hide resolved
mergin/test/test_client.py Outdated Show resolved Hide resolved
@MarcelGeo MarcelGeo merged commit 870b5b6 into master Sep 2, 2024
4 checks passed
@MarcelGeo MarcelGeo deleted the improve-client-error branch September 2, 2024 16:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wonder-sk wonder-sk wonder-sk left review comments

@MarcelGeo MarcelGeo MarcelGeo requested changes

@tomasMizera tomasMizera Awaiting requested review from tomasMizera

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@ValentinBuira @coveralls @wonder-sk @MarcelGeo @tomasMizera