Skip to content

Conversation

@stellamzr
Copy link
Contributor

@stellamzr stellamzr commented Apr 3, 2025

What I'm changing

  • replace request with httpx information in docstring
  • update docstrings
  • add readme with examples

resolves #63

Checklist

  • Tests pass: uv run pytest
  • Checks pass: uv run pre-commit --all-files

@stellamzr stellamzr requested a review from gadomski as a code owner April 3, 2025 12:56
@stellamzr stellamzr self-assigned this Apr 3, 2025
Copy link
Member

@gadomski gadomski left a comment

Choose a reason for hiding this comment

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

A couple of quick changes, and then a higher-level comment ... I've pushed a commit to this branch to add it to the mkdocs configuration ... this can be used to tell us if there's more things to fix up in the docstrings.

To run mkdocs:

uv run mkdocs serve

Comment on lines 14 to 26
## Currently Supported Endpoints

These endpoints are fully implemented and available in the current version of PySTAPI Client.

| Category | Endpoint | Description |
|----------|----------|-------------|
| Root | `/` | Root endpoint (for links and conformance) |
| Root | `/conformance` | Conformance information |
| Products | `/products` | List all products |
| Products | `/products/{product_id}` | Get specific product |
| Orders | `/orders` | List all orders |
| Orders | `/orders/{order_id}` | Get specific order |

Copy link
Member

Choose a reason for hiding this comment

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

I'm a little wary about including this table, as we'll have to keep it continually updated. I think I'd prefer to just point to the API docs, which will be generated from the code?

Comment on lines 31 to 32
Pre-request: The app should be accessible at `http://localhost:8000`.

Copy link
Member

Choose a reason for hiding this comment

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

Don't need this, since in the examples we're hitting a dummy endpoint.

Suggested change
Pre-request: The app should be accessible at `http://localhost:8000`.

Comment on lines 43 to 52
product = client.get_product(product_id="test-spotlight")

# List all Opportunities for a Product
opportunities = client.get_product_opportunities(product_id="test-spotlight")

# List orders
orders = client.get_orders()

# Get specific order
order = client.get_order(order_id="test-order")
Copy link
Member

Choose a reason for hiding this comment

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

Nit: let's make the call simpler, to show you don't have to pass by keyword:

Suggested change
product = client.get_product(product_id="test-spotlight")
# List all Opportunities for a Product
opportunities = client.get_product_opportunities(product_id="test-spotlight")
# List orders
orders = client.get_orders()
# Get specific order
order = client.get_order(order_id="test-order")
product = client.get_product("test-spotlight")
# List all Opportunities for a Product
opportunities = client.get_product_opportunities("test-spotlight")
# List orders
orders = client.get_orders()
# Get specific order
order = client.get_order("test-order")

@gadomski gadomski merged commit 82b7cfe into stapi-spec:main Apr 3, 2025
4 checks passed
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.

Update pystapi-client docstring and add them to this repo's docs

2 participants