8000 Added MVG errors with better error messages by OskarLiew · Pull Request #115 · vikinganalytics/mvg · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Added MVG errors with better error messages #115

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

Merged
merged 6 commits into from
Dec 7, 2021
Merged

Conversation

OskarLiew
Copy link
Contributor

Description

Added new MVG specific errors. In particular MVGAPIError that will automatically read the details of the error message

New dependencies: (Added to requirements_*.txt)

Fixes #43 (potential issues that are fixed by this PR)

Type of Change

  • Bug fix (non-breaking change which fixes an issue -> bump patch)
  • New feature (non-breaking change which adds functionality -> bump minor version)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected -> bump major version)
  • Documentation Update

Checklist

  • I have added tests that prove that my fix/feature works
  • Linters pass locally and I have followed PEP8 code style
  • New and existing tests pass locally
  • I have updated the documentation if needed
  • I have commented hard-to-understand areas in the code

Requirements

  • I have updated the MVG version

tuix
tuix previously requested changes Nov 30, 2021
Copy link
Contributor
@tuix tuix left a comment

Choose a reason for hiding this comment

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

beautifully implemented. Thanks for taking care of this. Just a small question about some functions

Comment on lines 25 to 31
def mock_fail_request(json_body):
return MockResponse(400, "Bad Request", json_body)


def assert_fail_response(msg, detail="No error message"):
assert msg == f"400 - Bad Request: {detail}"
8000
Copy link
Contributor

Choose a reason for hiding this comment

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

How are these two functions used? Do not seem to be invoked by other functions, and won't be called as a test function by pytest either?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

These were just something I was playing around with but decided was overkill. Seems I forgot to remove the functions

@OskarLiew OskarLiew requested a review from tuix December 1, 2021 14:52
@vnadhan
Copy link
Contributor
vnadhan commented Dec 3, 2021

Just found two minor issues in the documentation, otherwise good work @OskarLiew!

@vnadhan
Copy link
Contributor
vnadhan commented Dec 3, 2021

I tested this branch locally and it is now much easier to know what caused an error, however they are too lengthy with unwanted characters or detail. This is an error that happened when a source id has a space in it. Perhaps we should improve the errors we send out from the backend.
image

@OskarLiew OskarLiew requested a review from vnadhan December 6, 2021 12:25
vnadhan
vnadhan previously approved these changes Dec 6, 2021
@SergioMDCB SergioMDCB merged commit cea03f3 into master Dec 7, 2021
@SergioMDCB SergioMDCB deleted the proper-errors branch December 7, 2021 08:07
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.

Add more descriptive exceptions to MVG
4 participants
0