Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[ENH] Export Collection type directly #3178

Merged
merged 1 commit into from
Dec 17, 2024
Merged

Conversation

itaismith
Copy link
Contributor

Description of changes

Users want to use "Collection" for type annotations, but it is not being exported from the js-client.

Test plan

How are these changes tested?

  • Tests pass locally with pytest for python, yarn test for js, cargo test for rust

Documentation Changes

N/A

Copy link

Reviewer Checklist

Please leverage this checklist to ensure your code review is thorough before approving

Testing, Bugs, Errors, Logs, Documentation

  • Can you think of any use case in which the code does not behave as intended? Have they been tested?
  • Can you think of any inputs or external events that could break the code? Is user input validated and safe? Have they been tested?
  • If appropriate, are there adequate property based tests?
  • If appropriate, are there adequate unit tests?
  • Should any logging, debugging, tracing information be added or removed?
  • Are error messages user-friendly?
  • Have all documentation changes needed been made?
  • Have all non-obvious changes been commented?

System Compatibility

  • Are there any potential impacts on other parts of the system or backward compatibility?
  • Does this change intersect with any items on our roadmap, and if so, is there a plan for fitting them together?

Quality

  • Is this code of a unexpectedly high quality (Readability, Modularity, Intuitiveness)

@itaismith itaismith requested a review from jeffchuber November 20, 2024 23:08
@itaismith itaismith force-pushed the itai/fix-ts-type-exports branch from b7f6501 to b0e9f77 Compare November 23, 2024 01:17
@galkleinman
Copy link

Hi @itaismith, @jeffchuber, and thanks for this contribution @itaismith 💪

Any plans to merge and release this one anytime soon? The issue it solved kinda crashing our SDK (traceloop-sdk) and we're looking forward for this one to be merged.

@tazarov
Copy link
Contributor

tazarov commented Dec 12, 2024

@galkleinman, I think the failure in CI is due to an small scope creep related to #3216. Where we change the semantics of list_collection.

@itaismith, does it make sense to separate the two changes or is #3216 landing in the 0.5.x train alongside this one?

@itaismith
Copy link
Contributor Author

@tazarov We're going to land #3216 and this one right after!

@itaismith itaismith force-pushed the itai/fix-ts-type-exports branch from b0e9f77 to a175587 Compare December 17, 2024 01:56
@itaismith itaismith force-pushed the itai/fix-ts-type-exports branch from a175587 to cb6d9ed Compare December 17, 2024 02:52
@itaismith itaismith requested a review from jeffchuber December 17, 2024 17:12
@itaismith itaismith merged commit f2e637d into main Dec 17, 2024
77 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.

4 participants