Skip to content

Conversation

@ptgolden
Copy link
Collaborator

  • Adds type annotations throughout. There is one place where I have a type: ignore comment. It's because of an incomplete type annotation in the flatten_dict library

  • With the type annotations in place, moved the CLI interface from argparse to typer.

  • Adds discrete Stream classes (e.g. TSVStream, JSONStream, etc.) rather than having stream functions that all must take every single kwarg required for any stream. This makes things a little clearer: for example, a "JSONLStream" doesn't require a key_name parameter. It also allows tracking state on a stream object-- before, the operation of the TSV stream relied on tracking some global state attached to the function.

NOTE: I have not run these changes against the tests yet. There's probably some more work to do there.

* Adds type annotations throughout. There is one place where I have a
  `type: ignore` comment. It's because of an incomplete type annotation
  in the flatten_dict library

* With the type annotations in place, moved the CLI interface from
  argparse to typer.

* Adds discrete `Stream` classes (e.g. TSVStream, JSONStream, etc.)
  rather than having stream functions that all must take every single
  kwarg required for any stream. This makes things a little clearer: for
  example, a "JSONLStream" doesn't require a `key_name` parameter. It
  also allows tracking state on a stream object-- before, the operation
  of the TSV stream relied on tracking some global state attached to the
  function.

NOTE: I have not run these changes against the tests yet. There's
probably some more work to do there.
@ptgolden ptgolden marked this pull request as draft December 18, 2025 18:42
@ptgolden
Copy link
Collaborator Author

ptgolden commented Dec 18, 2025

Other small things I forgot to mention:

  • Moves from os.path to pathlib.Path
  • Make the signature of process_entities be named argument only, to prevent needing 10 different parameters all in the right position

Base automatically changed from map_tests_210 to main December 19, 2025 14:01
@amc-corey-cox amc-corey-cox marked this pull request as ready for review December 19, 2025 14:02
Copy link
Collaborator

@amc-corey-cox amc-corey-cox left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me.

@amc-corey-cox amc-corey-cox merged commit b7787cc into main Dec 19, 2025
2 checks passed
@amc-corey-cox amc-corey-cox deleted the map_tests_210-refactor branch December 19, 2025 18:59
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.

3 participants