Api spec rework #213
Api spec rework #213
Conversation
…en. This flag feels like a duct tape patch, and it feels like it directly competes with the openapi-generator without really improving on it
…nto api_spec_rework
| @@ -21,7 +21,7 @@ generate: install-generator ## Generate the SDK from our public openapi spec | |||
| -g python \ | |||
| -o ./generated \ | |||
| --additional-properties=packageName=groundlight_openapi_client | |||
| poetry run datamodel-codegen --input spec/public-api.yaml --output generated/model.py | |||
| poetry run datamodel-codegen --input spec/public-api.yaml --output generated/model.py --strict-nullable | |||
There was a problem hiding this comment.
What does --strict-nullable do? For less obvious flags its helpful to comment them or link to their documentation
There was a problem hiding this comment.
Yeah, let me add a comment in the file itself, but the useful discussion is here. For properties in the api spec that have nullable: true the generated pydantic class should let that property be Optional. For strange reasons, you need to set --strict-nullable for this to be true.
| description: "" | ||
| /v1/actions/detector/{detector_id}/rules: | ||
| $ref: '#/components/schemas/PaginatedRuleList' | ||
| description: '' | ||
| post: | ||
| operationId: Create rule |
There was a problem hiding this comment.
| operationId: Create rule | |
| operationId: Create detector rule |
There was a problem hiding this comment.
Just trying to add some parallelism to the name/description patterns
There was a problem hiding this comment.
I agreed with you at first, but then I realized this broke the parallelism to the delete rule endpoint. It's implicit that all rules belong to a detector
| get: | ||
| operationId: get notes | ||
| description: Retrieve notes for a detector | ||
| operationId: List rules |
There was a problem hiding this comment.
Which rules? All the rules for a user? Or a group?
There was a problem hiding this comment.
Users and groups should share the exact same set of rules, so I'm not sure if that should be differentiated
| if metadata is not None: | ||
| detector_creation_input.metadata = str(url_encode_dict(metadata, name="metadata", size_limit_bytes=1024)) | ||
| if confidence_threshold: |
There was a problem hiding this comment.
Was there a reason to reorder this? Not saying you need to change it just wondering if Im missing something
There was a problem hiding this comment.
No, I think I just happened to move it down while I was moving the pipeline_config default into the constructor
| from groundlight_openapi_client.model.note_request import NoteRequest | ||
| from groundlight_openapi_client.model.rule_request import RuleRequest | ||
| from groundlight_openapi_client.model.verb_enum import VerbEnum | ||
| from model import Detector, PaginatedRuleList, Rule |
There was a problem hiding this comment.
Any idea why this is still considered Experimental? Should we change that?
There was a problem hiding this comment.
At some point this moves into the main client body, but I think we just haven't had the impetus to change this yet. This was our way of leaving a door open in case we wanted to change things later
This is mainly the corresponding SDK update to the new process of generating the api spec. The generated code had been rotting for a while, so this is mainly cleaning that before we attach more onto the sdk. The next likely change (and slightly overdue) change is to make confidence non-nullable, which inherently is a small update to the api-spec |
We recently did a full re-haul of the public api spec. Functionality of the SDK should be unaffected, but this maps better onto what the service provides and sets up for improvements in the future.