Skip to content

Implement horizontal_spatial_domain factory - #51

Merged
benbart merged 3 commits into
mainfrom
bbarton/PR-7400
Apr 21, 2026
Merged

Implement horizontal_spatial_domain factory#51
benbart merged 3 commits into
mainfrom
bbarton/PR-7400

Conversation

@benbart

@benbart benbart commented Apr 21, 2026

Copy link
Copy Markdown
Contributor

Pull Request Checklist

I have:

  • performed a self review of my code I&A code style
    • Resources and Data Structures are sorted by ABC or a defined sorting pattern
  • updated the documentation accordingly
  • verified required action checks are passing
  • bumped the version number as appropriate

obj["Geometry"] = geometry
if zone_identifier is not None:
obj["ZoneIdentifier"] = zone_identifier
return obj

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.

Is there a reason to hold off on adding the orbit and track params and adding the oneOf validation? As it stands now this factory function can produce an empty dict which is invalid as far as the jsonschema goes. I would say if you're not going to add orbit then at least I'd make geometry required / raise a ValueError if geometry is not provided. But I think it would be just cleaner to implement the whole thing, it should be an additional like 10 lines of code.

Comment on lines +62 to +71
def horizontal_spatial_domain(
geometry: Optional[Geometry] = None,
zone_identifier: Optional[str] = None,
) -> HorizontalSpatialDomain:
obj: HorizontalSpatialDomain = {}
if geometry is not None:
obj["Geometry"] = geometry
if zone_identifier is not None:
obj["ZoneIdentifier"] = zone_identifier
return obj

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 would also keep the params in the order they are defined in the schema and build the resulting dict in that order when possible, and in this case it is! And I'd keep the formatting consistent with the implementations (empty line before the return statement).

All said and done if I was doing this PR I would commit this:

Suggested change
def horizontal_spatial_domain(
geometry: Optional[Geometry] = None,
zone_identifier: Optional[str] = None,
) -> HorizontalSpatialDomain:
obj: HorizontalSpatialDomain = {}
if geometry is not None:
obj["Geometry"] = geometry
if zone_identifier is not None:
obj["ZoneIdentifier"] = zone_identifier
return obj
def horizontal_spatial_domain(
zone_identifier: Optional[str] = None,
geometry: Optional[Geometry] = None,
# TODO(reweeden): Implement typing
orbit: Optional[dict[str, Any]] = None,
# TODO(reweeden): Implement typing
track: Optional[dict[str, Any]] = None,
) -> HorizontalSpatialDomain:
obj: HorizontalSpatialDomain = {}
if zone_identifier is not None:
obj["ZoneIdentifier"] = zone_identifier
if geometry is not None:
obj["Geometry"] = geometry
if orbit is not None:
obj["Orbit"] = orbit
if track is not None:
obj["Track"] = track
if orbit is None and track is None:
raise ValueError(
"one of 'orbit' or 'track' is required",
)
return obj

benbart and others added 2 commits April 21, 2026 13:08
Co-authored-by: Rohan Weeden <reweeden@alaska.edu>
@benbart
benbart merged commit 959a71e into main Apr 21, 2026
11 checks passed
@benbart
benbart deleted the bbarton/PR-7400 branch April 21, 2026 22:11
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