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.
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: method tools #28160
core: method tools #28160
Changes from all commits
0b371e5
57033db
ff7f148
e04f7b9
9185e91
066a832
b5daee7
ee66099
691f01b
0d5db79
9d8e15a
2def62d
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
this case actually doesn't matter - I think the one that matters is this
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 will be marking all the rest of the conversations surrounding this general issue as resolved. This thread here will be to discuss everything around the classmethod support.
Unfortunately, it seems that in order to get the desired behaviour the
@classmethod
decorator must be applied last. This is because we wantA.foo
to pass in the classA
as thecls
parameter. This behaviour does not seem to happen unless@classmethod
happens last.Here is what happens with making the
@tool
decorator return a method tool of theclassmethod
s underlying function:It is no longer a
classmethod
! Thecls
value does not get passed in as we would wish for it to.This can be fixed by making this simply apply the decorators in the desired order through some simple logic in
convert.py
:However this is not everything that we want since Python 3.13 still removed wrapping descriptors (like
property
s) withclassmethod
. So it would still not be compatible with 3.13 and above. For this reason I would be able to support the syntax you've requested but would not be able to support newer Python versions.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.
from all these differences, what are your thoughts on introducing a new decorator
tool_property
or something to distinguish these cases? Would we also need atool_classmethod
to be separate too?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.
Some additional thoughts:
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.
Having a separate decorator specifically for implementing this feature was always an idea, and is definitely the better solution.
To be honest, the reason things are implemented the way they currently are is because I thought that if I did not implement this in a way that satisfied the example code given by Eugene in the original issue then that issue would not be resolved.
Now that I know there is more leeway and freedom to implement this feature in the way that makes most sense rather than being locked into using the original tool decorator, that can absolutely be done.
How about I first create some sort of new decorator that is for method tools, and then from there I can see if classmethods are doable?
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.
sounds great!
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.
doing it as 2 separate PRs is also good practice. If you'd like to avoid double-work, you're welcome to put up a PR with just the unit tests first, just to make sure we're envisioning the same interface
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.
Check out my new PR #28695.
On top of doing what you suggested, I also came up with a much more elegant solution without any strange workarounds or shortcomings. The actual implementation is only something like 20 lines of added logic. Taking a second shot at this was definitely worth it.