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

fix: add default value in deprecated argument #1053

Merged
merged 1 commit into from
Jan 6, 2025

Conversation

Swalloow
Copy link
Contributor

@Swalloow Swalloow commented Dec 27, 2024

Added a default value of None to the trace_or_observation for optional usage.
It prevents potential confusion for users by clearly indicating the optional and deprecated parameter.


Important

Added default value None to trace_or_observation parameter in link() function in client.py to indicate it is optional and deprecated.

  • Function Signature:
    • In link() function in client.py, added default value None to trace_or_observation parameter.
    • This change clarifies that trace_or_observation is optional and deprecated.

This description was created by Ellipsis for 391d284. It will automatically update as commits are pushed.

@CLAassistant
Copy link

CLAassistant commented Dec 27, 2024

CLA assistant check
All committers have signed the CLA.

Copy link

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

Disclaimer: Experimental PR review

PR Summary

Improved parameter handling in the DatasetItemClient's link method by adding default value for deprecated parameter.

  • Modified langfuse/client.py to set default value of None for deprecated trace_or_observation parameter
  • Reordered parameter position in link method signature to place deprecated parameter last
  • Updated docstring to reflect parameter changes and deprecation status

💡 (1/5) You can manually trigger the bot by mentioning @greptileai in a comment!

1 file(s) reviewed, no comment(s)
Edit PR Review Bot Settings | Greptile

@marcklingen
Copy link
Member

Unfortunately, this was a positional argument in the original api here and later on the keyword arguments were added. @hassiebp can you review?

To me it seems like this requires a major bump as it is a breaking change. If yes, I’d suggest to add this in the next major version as this is clearly tech debt in this interface.

@marcklingen marcklingen requested a review from hassiebp January 3, 2025 16:43
Copy link
Contributor

@hassiebp hassiebp left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution!

@hassiebp hassiebp merged commit b8bca23 into langfuse:main Jan 6, 2025
1 check passed
@hassiebp
Copy link
Contributor

hassiebp commented Jan 6, 2025

Apologies, had to revert this PR here. As Marc has mentioned, the current interface expects positional arguments, such that moving the position of that argument would not be backwards compatible.

@Swalloow
Copy link
Contributor Author

Swalloow commented Jan 6, 2025

How should I fix this to include it in the next major version?

@hassiebp
Copy link
Contributor

@Swalloow - we have not yet created a v3 branch for which a PR such as this one could be merged into. We have created an internal task though with this PR linked in order to make sure this will be part of our next major release. Thanks again for your contribution!

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