Skip to content

Conversation

Copy link

Copilot AI commented Dec 18, 2025

Description

Implements reviewer feedback on typed objects API design:

SchemaProperty simplification

  • Removed default_factory parameter—pydantic TypeAdapter handles validation directly
  • Properties return None when absent rather than manufactured defaults
  • Updated tags and extensions to dict[str, str] | None and dict[str, Any] | None respectively

API additions

  • Added DownloadedObject.from_context() convenience method that extracts connector/cache from IContext
  • Marked BaseSpatialObject.compute_bounding_box() as @abstractmethod

Type system cleanup

  • Made extensions type explicit: dict[str, Any] instead of bare dict
  • Aligned property types with their BaseObjectData counterparts

Before:

class BaseObject:
    tags: dict[str, str] = SchemaProperty("tags", TypeAdapter(...), default_factory=dict)

After:

class BaseObject:
    tags: dict[str, str] | None = SchemaProperty("tags", TypeAdapter(dict[str, str] | None))

Checklist

  • I have read the contributing guide and the code of conduct

Warning

Firewall rules blocked me from connecting to one or more addresses (expand for details)

I tried to connect to the following addresses, but was blocked by firewall rules:

  • astral.sh
    • Triggering command: /usr/bin/curl curl -LsSf REDACTED grep -l om_context py rep (dns block)

If you need me to access, download, or install something from one of these locations, you can either:

Original prompt

Address the code review comments from SeequentEvo#164

Changes Required

1. Remove default parameter from jmespath expression method

Pydantic TypeAdapters can already handle default values. Remove the optional default parameter:

# Change from:
self, jmespath_expr: str, type_adapter: TypeAdapter[_T], default: _T | None = None
# To:
self, jmespath_expr: str, type_adapter: TypeAdapter[_T]

2. Simplify value handling in jmespath method

If relying on pydantic to handle default values, simplify the code to:

if isinstance(value, (jmespath.JMESPathArrayProxy, jmespath.JMESPathObjectProxy)):
    value = value.raw
return self._typed_adapter.validate_python(value)

3. Raise error for .json extension

Instead of auto-appending .json to the path, raise a ValueError:

raise ValueError("`path` must end in `.json`.")

4. Add from_context() method to DownloadedObject

Add a factory classmethod from_context() to the DownloadedObject class.

5. Rename create_async to create

Change the method name from create_async to just create:

async def create(

6. Update the _create call to use await

return await object_type._create(context, data, parent, path)

7. Mark appropriate method as abstract

Add @abstractmethod decorator where indicated by the reviewer.

8. TypeAdapter naming nitpick

Rename the bounding box type adapter:

_bbox_type_adapter: ClassVar[TypeAdapter[BoundingBox]] = TypeAdapter(BoundingBox)

9. Consider accessing DownloadedObject internals

For the IContext requirement, consider accepting access to DownloadedObject._connector and DownloadedObject._cache instead of requiring the full IContext.

Please find the relevant files in the typed-objects branch and apply these code review suggestions appropriately.

This pull request was created from Copilot chat.


💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.

Copilot AI and others added 2 commits December 18, 2025 03:46
…ext, mark abstract method

Co-authored-by: BenLewis-Seequent <7391596+BenLewis-Seequent@users.noreply.github.com>
Co-authored-by: BenLewis-Seequent <7391596+BenLewis-Seequent@users.noreply.github.com>
Copilot AI changed the title [WIP] Address code review comments for jmespath expression method Address code review feedback from PR #164 Dec 18, 2025
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