Skip to content

update to CatalogNamespace#98

Merged
ryanraaschCDC merged 12 commits into
mainfrom
rr-68-type-hints
Jun 30, 2026
Merged

update to CatalogNamespace#98
ryanraaschCDC merged 12 commits into
mainfrom
rr-68-type-hints

Conversation

@ryanraaschCDC

Copy link
Copy Markdown
Collaborator

This includes the same updates as in #68 but is up to date with the main branch.

@ryanraaschCDC ryanraaschCDC linked an issue Jun 24, 2026 that may be closed by this pull request
@ryanraaschCDC ryanraaschCDC requested a review from Copilot June 24, 2026 14:05

Copilot AI 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.

Pull request overview

This PR improves static typing support for the dynamic datacat / reportcat catalog namespaces so editors can better infer types (per Issue #68), by marking the package as typed and introducing a typed namespace wrapper.

Changes:

  • Add py.typed and package configuration to ship type information in sdists/wheels.
  • Introduce CatalogNamespace (a SimpleNamespace subclass) and update catalog construction to use it.
  • Add TYPE_CHECKING attribute declarations to help editors resolve endpoint methods on dynamic access chains.

Reviewed changes

Copilot reviewed 3 out of 4 changed files in this pull request and generated 1 comment.

File Description
pyproject.toml Ensures py.typed is included in built distributions.
MANIFEST.in Includes py.typed in source distributions.
cfa/dataops/py.typed Marks cfa.dataops as a typed package (PEP 561).
cfa/dataops/catalog.py Adds CatalogNamespace + typing annotations to improve type inference for datacat chains.
Comments suppressed due to low confidence (1)

cfa/dataops/catalog.py:772

  • dict_to_sn types defaults as optional (dict | None) but immediately passes it to DatasetEndpoint (which expects a dict) and later indexes into it (defaults["storage"][...]). Add an early check to narrow the type (and raise a clear error) or make defaults non-optional in the signature/docstring.
def dict_to_sn(d: Any, defaults: dict | None = None, ns: str = "") -> CatalogNamespace:
    """Simple recursive namespace construction

    Args:
        d (Any): a dict, list or other

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread cfa/dataops/catalog.py Outdated
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
@ryanraaschCDC ryanraaschCDC requested a review from damonbayer June 24, 2026 14:11
@ryanraaschCDC ryanraaschCDC marked this pull request as ready for review June 25, 2026 20:02
@damonbayer

damonbayer commented Jun 25, 2026

Copy link
Copy Markdown
Contributor

Is this working for you locally? My editor isn't picking it up.

I tried an alternative approach in #109, which seems more appropriate with the cost of being much more complicated.

@ryanraaschCDC

Copy link
Copy Markdown
Collaborator Author

I don't think mine is picking it up either. I'm not as familiar with using type hints so I was copying over your other code with the most recent version of the main branch. I like your new PR. That might be the way to go.

@damonbayer

Copy link
Copy Markdown
Contributor

I don't think mine is picking it up either. I'm not as familiar with using type hints so I was copying over your other code with the most recent version of the main branch. I like your new PR. That might be the way to go.

I merged it in here

@ryanraaschCDC

Copy link
Copy Markdown
Collaborator Author

this is working great for me now

@ryanraaschCDC

Copy link
Copy Markdown
Collaborator Author

@damonbayer is this working for you? I think we're good to merge

@damonbayer

Copy link
Copy Markdown
Contributor

@ryanraaschCDC fine by me! Feels a bit fragile but I think it's worthy of trying more seriously.

@ryanraaschCDC

Copy link
Copy Markdown
Collaborator Author

could you approve this PR then? Thanks!

@ryanraaschCDC ryanraaschCDC merged commit e92b3b8 into main Jun 30, 2026
4 checks passed
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.

Type hints aren't resolved correctly

4 participants