-
Notifications
You must be signed in to change notification settings - Fork 16.3k
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
core: improved method tools #28695
Closed
+217
−2
Closed
core: improved method tools #28695
Changes from all commits
Commits
Show all changes
9 commits
Select commit
Hold shift + click to select a range
883d120
Add method tools
ethanglide ee50e43
Merge remote-tracking branch 'upstream/master'
ethanglide 6b881cc
Move work away from StructuredTool
ethanglide af1a44c
Add correct metadata
ethanglide 3565b9f
Add wrapper decorator for MethodTool class
ethanglide 7272bb5
Merge remote-tracking branch 'upstream/master'
ethanglide 57118fe
Add nonstandard parameter tests
ethanglide fca11c4
Add tool info assertions to all method tool tests
ethanglide 6a01806
Merge branch 'master' into master
ethanglide File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains 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
This file contains 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
ethanglide marked this conversation as resolved.
Show resolved
Hide resolved
|
This file contains 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
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we make this a function instead of a class?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see - this maybe solves the issue of typing. going to suggest some ideas - not sure if they work
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The class is needed for classmethod support. I have created a wrapper function
methodtool
that simply returnsMethodTool(func)
. How's that?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why is it needed for classmethod support?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mentioned in another reply:
Keep in mind that in your other solution (a function that returns a
property
), you are still returning a descriptor class. That class,property
. works for our regular method tool case, but it will not work for class methods. Notice in theMethodTool
class that it will pass inowner
(class itself) forouter_instance
with classmethods, andinstance
(class instance) for regular methods. Since theproperty
class is not enough for our use, I had to create a custom descriptor that did exactly what we wanted it to."Why could we not do
classmethod(property(func))
for classmethods?" you may ask. That is becauseclassmethod
no longer wraps descriptors as of Python 3.13.If, for syntax reasons, you would like the decorator to be a function that is something that I have added to the code. The function is simply a wrapper for the
MethodTool
class since that overloaded__get__
method is needed, but theMethodTool
class never needs to be exposed to client code.