Skip to content

Fix by_name() raising TypeError instead of returning None when nothing matches - #133

Open
keeltrace wants to merge 1 commit into
HumanBrainProject:masterfrom
keeltrace:fix/by-name-none-on-no-match
Open

Fix by_name() raising TypeError instead of returning None when nothing matches#133
keeltrace wants to merge 1 commit into
HumanBrainProject:masterfrom
keeltrace:fix/by-name-none-on-no-match

Conversation

@keeltrace

@keeltrace keeltrace commented Aug 24, 2026

Copy link
Copy Markdown

AI Fully Automated PR - Review With Care

Summary

by_name() on a KGObject subclass without a client (the local
instance-library path used by controlled terms) raised
TypeError: object of type 'NoneType' has no len() when the requested
name did not match any instance, instead of returning None.

Reproduction (fairgraph master at time of fix; no KG connection needed):

from fairgraph.openminds.v5.controlled_terms import Species
Species.by_name("definitely not a species")
# TypeError: object of type 'NoneType' has no len()

Root cause

In fairgraph/kgobject.py, the instance-library branch of by_name()
used _instance_lookup.get(name, None). When nothing matched, objects
was set to None, and the following if len(objects) == 0: check
dereferenced it. The "contains" branch already initialised an empty
list, so only exact matches were affected.

Fix

One line: default to an empty list — cls._instance_lookup.get(name, [])
— so the existing "no match → return None" path works as documented and
matches the behaviour of from_id() with an unknown id.

Tests

New offline regression test file test/test_by_name.py (no network or
KG credentials required), covering:

  • no-match returns None (regression for this issue)
  • match="contains" with no match returns None
  • single match is returned
  • multiple matches warn and return the first; all=True returns all
  • invalid match value raises ValueError

Verified that the new test fails with the exact reported TypeError
when the one-line fix is reverted, and passes with it applied.
Full suite: 88 passed, 57 skipped (KG-connection tests), 0 failures.

Fixes #130

…g matches

In the no-client instance-library path, _instance_lookup.get(name, None)
left objects as None when the name had no match, and the subsequent
len(objects) raised TypeError. Return an empty list as the default
instead, so by_name() correctly returns None.

Fixes HumanBrainProject#130
@keeltrace

Copy link
Copy Markdown
Author

AI generated Maintainer review summary for Fairgraph PR #133. Summary reviewed and approved for posting by Keeltrace.

  • Code fix is correct and merge-worthy. The one-line change from .get(name, None) to .get(name, []) cleanly fixes the crash and preserves existing API behavior.
  • Tests are good. They cover the regression plus normal hit, contains, duplicate, all=True, and invalid-match behavior.
  • No code changes requested. The implementation can be approved as-is.
  • Upstream CI has not actually run yet. GitHub Actions is currently action_required, so a maintainer needs to authorize it before merge.
  • Two PR-description corrections are advisable:
    • The reproduction says fairgraph 0.14.0 but uses a v5 import that does not exist in 0.14.0.
    • get_by_id() should likely say from_id().
  • There is a separate pre-existing wording bug where the error says "exact" instead of "equals", but this PR should not be expanded to fix it.

Overall: approve after CI passes; only clean up the PR description.

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.

by_name() raises TypeError instead of returning None when nothing matches

1 participant