search

DataJunction / dj

A metrics platform.
http://datajunction.io
MIT License
34 stars 15 forks source link

chore remove try/except in client methods #987

Closed anhqle closed 4 months ago

anhqle commented 4 months ago

Summary

Since the djclient._session already try/except to propagates any error returned by server, the client methods don't need to

Test Plan

anhqle commented 4 months ago

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @anhqle and the rest of your teammates on Graphite Graphite

netlify[bot] commented 4 months ago

Deploy Preview for thriving-cassata-78ae72 canceled.

Name Link
Latest commit cf1c25c9f4ea8a2ae1bc56340bd812c08f572596
Latest deploy log https://app.netlify.com/sites/thriving-cassata-78ae72/deploys/663dc91863f9570008026e7c
shangyian commented 4 months ago

@anhqle I think you'll need to run make check from clients/python to run the linters

anhqle commented 4 months ago

@anhqle I think you'll need to run make check from clients/python to run the linters

The CI failed from genuine test failures actually. These tests expect the client to raise (the mock server doesn't).

I removed the tests in the latest commit. We can discuss in person if you want to keep the test and mock the server so that it raises.

shangyian commented 4 months ago

@anhqle Hmm, let's not mock any behavior in the server. One of the nicer things about the current setup is that the client tests are always running against the latest version of the server, so we can be sure of compatibility. I think we should just change the test to not expect the client to raise.

anhqle commented 4 months ago

I left the try/except code in compile.py intact since that code doesn't just raise (which is already taken care of by server), but also saves the error in an object to be printed out.

Unclear how important is the error saving functionality, so I didn't rip it out.