feat(generators): add --exclude-external-imports flag#2
feat(generators): add --exclude-external-imports flag#2
Conversation
e891c43 to
5df92ed
Compare
5df92ed to
ff5ed11
Compare
🔍 Adversarial Review — PR #2SummarySolid feature with a clear, well-documented use-case (JSON-LD 1.1 Verdict: Approve with suggestions — no blocking bugs found, but the test suite should be strengthened. 🐛 Bugs & IssuesAfter thorough analysis, no hard bugs were identified. Several initially suspicious areas were verified safe:
|
| Gap | Risk | Recommendation |
|---|---|---|
No test that linkml:types slots (e.g. string, date) survive filtering |
Medium | Add assertion that standard linkml types used by local slots still produce correct @type coercions |
mergeimports=False parametrization doesn't verify "no effect" claim |
Low | The test asserts external terms are absent, but doesn't verify the output is identical with and without the flag. Add a comparison: output_with_flag == output_without_flag when mergeimports=False |
No test combining both exclude_imports=True and exclude_external_imports=True |
Low | Verify no crash and that exclude_imports dominates |
| No CLI invocation test | Low | Add a CliRunner test that passes --exclude-external-imports and verifies it reaches the generator |
| No test for empty external schema | Low | Edge case: external schema with no classes/slots should produce empty sets, not errors |
| No test for local file import preservation | Medium | Add a second local-file import (not URL-based) and verify its elements ARE included |
| No test for transitive external imports | Low | External schema A imports external schema B — verify B's elements are also excluded |
📋 Fix Plan
Before merge (recommended):
-
Add a local-file-import preservation test. The current test only has one URL import. Add a local file import alongside it and assert its elements are included. This is the core differentiator from
exclude_importsand should be tested explicitly. -
Add
mergeimports=Falseequivalence assertion. Whenmergeimports=False, generate output both with and withoutexclude_external_importsand assert they're identical. This directly tests the docstring's "no effect" claim. -
Add
linkml:typessurvival assertion. In the existing test, assert that a local slot withrange: stringstill gets correct@typecoercion (e.g."xsd:string"), proving standard imports aren't filtered.
Post-merge / follow-up (nice-to-have):
-
Expand the docstring to note that
http(s)://is the detection heuristic (not arbitrary URIs), and that external prefixes are intentionally preserved. -
Consider a
click.UsageErroror warning when both--exclude-importsand--exclude-external-importsare set. -
Add CLI roundtrip test via
CliRunner.
✅ What's Good
- Clean design — follows the established
exclude_importspattern exactly, making the code predictable and maintainable. - Correct
schema_maptraversal — the static method is well-structured, idempotent-safe, and handles the transitive closure correctly. - Excellent docstring — clearly explains the
@protecteduse-case, the interaction withmergeimports=False, and the scope of the filtering. - Backward compatible — defaults to
False, zero behavior change for existing users. - CLI integration — proper boolean flag pair with
--no-prefix for explicit opt-out.
Add a dedicated --exclude-external-imports / --no-exclude-external-imports CLI flag to control whether external vocabulary terms are included in generated artifacts when --no-mergeimports is set. Previously external terms leaked into JSON-LD contexts even with --no-mergeimports. The new flag explicitly suppresses terms whose class_uri or slot_uri belong to an imported (external) schema. Tests cover linkml:types built-in import preservation, local file import preservation, and interaction with mergeimports=False. Signed-off-by: jdsika <carlo.van-driesten@bmw.de>
8553afa to
d1adeeb
Compare
The docstring incorrectly stated that the flag has no effect when mergeimports=False. In reality, external vocabulary elements can still leak into the context via the schema_map even in that mode, and the flag actively catches them — as verified by the existing test test_exclude_external_imports_works_with_mergeimports_false. Replace the inaccurate note with a correct statement. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…untime Add support for dict-based slots, dict-of-dicts slot_usage, and list slot_usage in add_class(). Move general SchemaBuilder tests from linkml to linkml_runtime test suite.
…-for-search-bar-optimtimizations Jinja template changes for documentation pages search bar optimizations
* test: reduce issue snapshot file count first pass * test: migrate four issue snapshot cases to target suites * test: fold migrated issue cases into target suites
Make gen_doc output ordering deterministic
Updating the Community Meeting schedule to have the most up to date information for the April 19 and May 21, 2026 community call presenters. Also took the opportunity to remove the disabled Slack join link from the page.
Update Community-Meetings.md
Summary
Add a
--exclude-external-importsflag to the JSON-LD context generator that suppresses terms from URL-based external vocabulary imports, keeping only locally defined classes and slots.Problem
When a schema imports an external vocabulary via URL (e.g. W3C Verifiable Credentials), all imported terms leak into the generated JSON-LD context — even with
--no-mergeimports. This is problematic when the external vocabulary defines@protectedterms in its own context (JSON-LD 1.1 §4.1.11): redefining them locally violates the protection semantics.Solution
When
--exclude-external-importsis set, the context generator identifies classes and slots from HTTP(S)-based imports viaSchemaView.schema_mapand skips them duringvisit_class/visit_slot. Local file imports and linkml standard imports are preserved.Changes
jsonldcontextgen.py: Addexclude_external_importsfield,_collect_external_elements()static helper, and filtering invisit_class()/visit_slot(). Add CLI option.test_jsonldcontextgen.py: Add parametrized test covering both merge modes, verifying local elements preserved and external elements excluded.Backward Compatibility
The flag defaults to off — existing behavior is fully preserved.