NSLS-II / olog

Python client to the Olog

BSD 3-Clause "New" or "Revised" License

2 stars 4 forks source link

New Client API of Olog by httpx #10

Closed ke-zhang-rd closed 4 years ago

ke-zhang-rd commented 4 years ago

Description

This PR try to satisfy new olog API here

Motivation and Context

Issues https://github.com/NSLS-II/pyolog2/issues/7 https://github.com/NSLS-II/pyolog2/issues/9

How Has This Been Tested?

Script:

from olog import Client
cli = Client(<HTTP_ADDRESS>, <USER>, <PASSWORD>)
cli.<REST_API_CALL>

danielballan commented 4 years ago

This is a looking good. Do you think we should. move ahead without waiting for a script or container for deploying the server on CI by relying on VCR?

If you still have access to a test server, you could run the test suite there and commit the cassettes generated by VCR. Then the tests can run on CI and we only need to rerun them against a test server when we make an intentional change that alters the HTTP requests, which would be rare in maintenance.

ke-zhang-rd commented 4 years ago

This is a looking good. Do you think we should. move ahead without waiting for a script or container for deploying the server on CI by relying on VCR?

Yes, and I assume you realize we are relying on a not merged vcrpy PR for supporting httpx, here. I generally feel that's fine.

If you still have access to a test server, you could run the test suite there and commit the cassettes generated by VCR. Then the tests can run on CI and we only need to rerun them against a test server when we make an intentional change that alters the HTTP requests, which would be rare in maintenance.

That's exactly what I'm gonna do. But before record cassettes, I'm waiting some issue debug on server side.

danielballan commented 4 years ago

Yep, using that branch seems fine at this stage. I left some comments on #12 to try to help un-block your progress.

ke-zhang-rd commented 4 years ago

@danielballan @mrakitin @shroffk I emphasised #12, updated test and made a new vcrpy record. It should be ready for final review.

codecov-commenter commented 4 years ago

Codecov Report

:exclamation: No coverage uploaded for pull request base (master@30997cc). Click here to learn what that means. The diff coverage is n/a.

@@            Coverage Diff            @@
##             master      #10   +/-   ##
=========================================
  Coverage          ?   95.67%           
=========================================
  Files             ?        2           
  Lines             ?      208           
  Branches          ?        0           
=========================================
  Hits              ?      199           
  Misses            ?        9           
  Partials          ?        0

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 30997cc...cafc2bf. Read the comment docs.