Collapse map-data onto a single writer API - #307
Open
amc-corey-cox wants to merge 1 commit into
Open
Conversation
map-data used two different writer stacks depending on whether -O was passed: the multi-output path built a MultiStreamWriter, while the single-output path used the function-based writers plus a hand-rolled header-rewrite block that depended on tsv_stream/csv_stream stashing state on the function object. Route both through MultiStreamWriter — the single-output case is just the multi-output case with no additional outputs. MultiStreamWriter already handles the tabular header rewrite generically, so the inline rewrite and the get_stream_writer/rewrite_header_and_pad/nullcontext/os imports are gone. The function-based API (yaml_stream, json_stream, jsonl_stream, tsv_stream, csv_stream, get_stream_writer, TabularStreamWriter.stream) is now unused internally. Rather than delete exported names outright, deprecate them with DeprecationWarning matching the repo's existing "removed in a future version" style. Adds a CLI-level test for tabular output over heterogeneous records, which previously had no coverage.
Contributor
There was a problem hiding this comment.
Pull request overview
Unifies the map-data CLI output pipeline by routing both single-output and -O/--additional-output cases through the class-based MultiStreamWriter, while keeping the legacy function-based writer API available but deprecated (per #302). This reduces duplicated writer logic and centralizes tabular header-rewrite behavior.
Changes:
- Refactor
map-datastreaming output to always useMultiStreamWriter(single output becomes the degenerate multi-output case). - Deprecate function-based writer entry points (
*_stream,get_stream_writer,TabularStreamWriter.stream) by emittingDeprecationWarning, and adjust tests to either assert or silence warnings as appropriate. - Add a CLI-level TSV regression test covering heterogeneous records (late-discovered tabular columns) with order-independent assertions.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
src/linkml_map/cli/cli.py |
Removes the separate single-output writer path and routes all streaming output through MultiStreamWriter. |
src/linkml_map/writers/output_streams.py |
Adds a shared deprecation-warning helper and deprecates legacy function API + TabularStreamWriter.stream. |
tests/test_cli/test_cli_tabular.py |
Adds an integration test ensuring TSV output rewrites/pads correctly when columns are discovered late. |
tests/test_writers/test_output_streams.py |
Updates tests to handle deprecations (silence in legacy tests, assert warnings in deprecation tests). |
Comments suppressed due to low confidence (1)
src/linkml_map/writers/output_streams.py:469
csv_stream()retainskey_namefor API compatibility but no longer uses it after switching towriter.process(chunks). To prevent Ruff unused-argument warnings (ARG001), mark the parameter with# noqa: ARG001(or rename to_key_namein a future breaking release).
"""
Convert chunks of objects to CSV format.
.. deprecated::
Use ``make_stream_writer(OutputFormat.CSV)`` and drive it via
Comment on lines
440
to
+444
| """ | ||
| Convert chunks of objects to TSV format. | ||
|
|
||
| .. deprecated:: | ||
| Use ``make_stream_writer(OutputFormat.TSV)`` and drive it via |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #302.
map-dataused two different writer stacks depending on whether-Owas passed. The-Opath built aMultiStreamWriter; the single-output path used the function-based writers and a hand-rolled header-rewrite block that only worked becausetsv_stream/csv_streamstash their headers on the function object.This routes both through
MultiStreamWriter— the single-output case is just the multi-output case with no additional outputs.MultiStreamWriteralready handles the tabular header rewrite generically, so the inline rewrite and theget_stream_writer/rewrite_header_and_pad/nullcontext/osimports drop out.The function-based API (
yaml_stream,json_stream,jsonl_stream,tsv_stream,csv_stream,get_stream_writer, and theTabularStreamWriter.streamalias) is now unused internally. Per the decision on #302 it is deprecated, not deleted: the names stay exported fromlinkml_map.writers, now emitting aDeprecationWarningthat matches the repo's existing "removed in a future version" wording. The raw line reduction lands later when the deprecation cycle completes.Testing
TestDeprecatedFunctionAPIasserts each deprecated name warns; the redundant function-API tests are marked to ignore the warning so the suite stays quiet.Note for reviewers
While writing the CLI test I hit a pre-existing quirk unrelated to this change: tabular column order varies by environment (subprocess CLI emits insertion order, in-process
CliRunnerunder pytest emits sorted order). The object keys and values are identical either way — only column ordering differs, which is cosmetic. The new test is order-independent because of it. Possibly worth its own issue.