Skip to content
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

Correct generated schema for Reference types #522

Merged
merged 3 commits into from
Jun 25, 2024
Merged

Conversation

tpoliaw
Copy link
Contributor

@tpoliaw tpoliaw commented Jun 25, 2024

I think this resolves the plan schemas returned from the plans endpoint. Do we
want to include some field in the schema to mark that the field should be a
reference rather than the value itself?

When bluesky protocol types are translated into references to allow them
to be passed by name from external processes, the generated schema needs
to specify the type that the reference should refer to rather than the
type of the value itself (str).

The previous version used the field name as the key for this entry in
the schema instead of the constant 'type' used and expected elsewhere.
Copy link

codecov bot commented Jun 25, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 91.06%. Comparing base (1cc2b8a) to head (46598b0).

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #522   +/-   ##
=======================================
  Coverage   91.06%   91.06%           
=======================================
  Files          40       40           
  Lines        1779     1779           
=======================================
  Hits         1620     1620           
  Misses        159      159           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@tpoliaw
Copy link
Contributor Author

tpoliaw commented Jun 25, 2024

Should the type value change as well? The <class 'bluesky.protocols.Readable'> form is not that useful - would it be better as bluesky.protocols.Readable?

Instead of using the repr(cls) output. Gives

    bluesky.protocols.Readable

instead of

    <class 'bluesky.protocols.Readable'>
Copy link
Contributor

@callumforrester callumforrester left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

Do we
want to include some field in the schema to mark that the field should be a
reference rather than the value itself?

Possibly, but let's do that in another PR, could also be done as part of #518

@DiamondJoseph
Copy link
Contributor

We can infer it from whether the type is a known type or otherwise referenced in the schema.

Any time we have as a plan parameter a BaseModel/Dataclass/TypedDict, I think the generated schema will include $ref and the full definition of that type (although we should check). Would we ever be passing in something that is not either a default JSON type, a BaseModel/Dataclass/TypedDict or a device reference?

@DiamondJoseph
Copy link
Contributor

Opened #523 to support the case of references nested inside structure, and having tested that TypedDict (and likely others) correctly get a $ref and defined subschema, this should allow us to handle #134

@tpoliaw tpoliaw merged commit f0248fb into main Jun 25, 2024
24 checks passed
@tpoliaw tpoliaw deleted the reference_schema branch June 25, 2024 14:42
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.

3 participants