Skip to content

fix: Better handling for optional arguments - #1124

Merged
DiamondJoseph merged 5 commits into
mainfrom
better_null_handling
Jul 14, 2025
Merged

DiamondJoseph merged 5 commits into
mainfrom
better_null_handling

Conversation

@DiamondJoseph

Copy link
Copy Markdown
Contributor

To be merged into main after #1122 rather than into #1122 just arranged for convenient review.

Adjusts handling of optional arguments in plan parameters to use the required field from the JsonSchema only instead of additionally explicitly allowing null as an argument. This makes the schema easier to read and automated tools should work better with it.

@DiamondJoseph
DiamondJoseph requested a review from a team as a code owner July 14, 2025 14:03
@DiamondJoseph
DiamondJoseph marked this pull request as draft July 14, 2025 14:08
@DiamondJoseph
DiamondJoseph force-pushed the better_null_handling branch from 276c168 to f2601b3 Compare July 14, 2025 14:15
@codecov

codecov Bot commented Jul 14, 2025 •

Copy link
Copy Markdown

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 94.48%. Comparing base (22b287d) to head (cfd9e6d).
Report is 1 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1124   +/-   ##
=======================================
  Coverage   94.48%   94.48%           
=======================================
  Files          41       41           
  Lines        2555     2557    +2     
=======================================
+ Hits         2414     2416    +2     
  Misses        141      141           

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@DiamondJoseph
DiamondJoseph force-pushed the better_null_handling branch from f2601b3 to 9cc49b6 Compare July 14, 2025 14:29
@DiamondJoseph
DiamondJoseph force-pushed the better_null_handling branch from 9cc49b6 to 5842edf Compare July 14, 2025 15:02
"radius": {
"description": "Radius of the circle",
"exclusiveMinimum": 0,
"exclusiveMinimum": 0.0,

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I do not know why these changes have appeared here just from adjusting handling of | None, as they do not allow None or defaults... I have always been confused why they got set to integer 0 instead of float 0.0, and I think this is more correct but why!?

StrictConfig: ConfigDict = {"extra": "forbid", "arbitrary_types_allowed": True}

@dataclass(config=StrictConfig)
class Circle(Region[Axis]):
      [...]
      radius: float = Field(description="Radius of the circle", gt=0.0)

@DiamondJoseph
DiamondJoseph marked this pull request as ready for review July 14, 2025 15:13

@tpoliaw tpoliaw left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'm not sure what I think of int | None being treated differently to int | None = None but I can't think of a better way and this does make the schema more useful in the majority of cases.

@DiamondJoseph

DiamondJoseph commented Jul 14, 2025 •

Copy link
Copy Markdown
Contributor Author

I'm not sure what I think of int | None being treated differently to int | None = None but I can't think of a better way and this does make the schema more useful in the majority of cases.

int | None = None -> This argument is optional, if you provide it it must be an int
int | None -> This argument is required and can be an int or None

Can I think of any examples where they are using the 2nd and shouldn't actually be using the first? Not right now. Not giving None a privileged default?

But if that is what your plan should have, cutting the possibility of passing None means you must pass an int.

Base automatically changed from update-deps to main July 14, 2025 17:39
@DiamondJoseph
DiamondJoseph merged commit 662a485 into main Jul 14, 2025
@DiamondJoseph
DiamondJoseph deleted the better_null_handling branch July 14, 2025 17:47
callumforrester added a commit to DiamondLightSource/visr-ui that referenced this pull request Jul 16, 2025
Create a dropdown menu for selecting a plan, populate using a mocked
call to blueapi using mockserviceworker.

Edit mocked output assuming that the following have been fixed:
- DiamondLightSource/blueapi#1129
- DiamondLightSource/blueapi#1123
- DiamondLightSource/blueapi#1124

And reorder the scanspec schema so that its self-referential components
can be detected in a single pass.
callumforrester added a commit to DiamondLightSource/visr-ui that referenced this pull request Jul 17, 2025
Create a dropdown menu for selecting a plan, populate using a mocked
call to blueapi using mockserviceworker.

Edit mocked output assuming that the following have been fixed:
- DiamondLightSource/blueapi#1129
- DiamondLightSource/blueapi#1123
- DiamondLightSource/blueapi#1124

And reorder the scanspec schema so that its self-referential components
can be detected in a single pass.
callumforrester added a commit to DiamondLightSource/visr-ui that referenced this pull request Jul 22, 2025
Create a dropdown menu for selecting a plan, populate using a mocked
call to blueapi using mockserviceworker.

Edit mocked output assuming that the following have been fixed:
- DiamondLightSource/blueapi#1129
- DiamondLightSource/blueapi#1123
- DiamondLightSource/blueapi#1124

And reorder the scanspec schema so that its self-referential components
can be detected in a single pass.
callumforrester added a commit to DiamondLightSource/visr-ui that referenced this pull request Jul 22, 2025
Create a dropdown menu for selecting a plan, populate using a mocked
call to blueapi using mockserviceworker.

Edit mocked output assuming that the following have been fixed:
- DiamondLightSource/blueapi#1129
- DiamondLightSource/blueapi#1123
- DiamondLightSource/blueapi#1124

And reorder the scanspec schema so that its self-referential components
can be detected in a single pass.
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.

2 participants