Add method to get Method by name #345
Conversation
|
This feature makes sense, but I think it would be more useful as a method on the It looks like you've properly handled the issue where two methods have the same name, which was my primary concern. All the SDKs would benefit from a method like this. |
barnjamin
requested review from
tzaffi and
ahangsu
and removed request for
michaeldiamant
run_integration.sh
Outdated
|
|
||
| ## Copy feature files into the project resources | ||
| mkdir -p test/features | ||
| cp -r test-harness/features/* test/features | ||
|
|
||
| exit | ||
|
|
There was a problem hiding this comment.
will revert once things look ok
barnjamin
force-pushed
the
get-method-by-name
branch
from
c17dc9c
to
b4cff48
Compare
Co-authored-by: Zeph Grunschlag <tzaffi@users.noreply.github.com>
algosdk/abi/interface.py
Outdated
| @@ -49,3 +49,23 @@ def undictify(d: dict) -> "Interface": | |||
| method_list = [Method.undictify(method) for method in d["methods"]] | |||
| desc = d["desc"] if "desc" in d else None | |||
| return Interface(name=name, desc=desc, methods=method_list) | |||
|
|
|||
| def get_method_by_name(self, name: str) -> Method: | |||
| methods_filtered = list( | |||
There was a problem hiding this comment.
similar bias here.... general question, and not for this PR. Seems like there's a lot of overlap between contract and interface. Having a common abstract supertype might make sense.
There was a problem hiding this comment.
I think a change like this is a little outside of the scope of this PR, but it is something we should consider.
I definitely dislike how much code is repeated between the two methods though. Perhaps a short term solution is to define a get_method_by_name(methods: list[Method], name: str) -> Method helper function which implements the logic, then call into it from both Interface.get_method_by_name and Contract.get_method_by_name? I think all SDKs would benefit from this @barnjamin
There was a problem hiding this comment.
If itll be abstracted in another PR anyway, I'm inclined to leave it as is for now unless it makes you want to puke
There was a problem hiding this comment.
I've not thought through all the implications of making Contract inherit from Interface, so I'm not sure if/when we'd want to make that change. In an effort to not hold up these PRs because of that, I think they should be mergable on their own, which means trying to avoid duplicate code.
From a maintenance perspective there's way fewer opportunities for problems if the function is implemented in just one place.
There was a problem hiding this comment.
Makes sense, just added it
There was a problem hiding this comment.
Has testing been conducted using these steps against the scenarios in the accompanying SDK testing PR?
It's possible to merge the other one first, assuming the suggestion to add a special tag to the scenarios such as @abi.contract is taken.
Yes, the green check on |
Got it. So since the SDK Testing PR has 3 approvals, once you're confident that the tag trick works, it would be great if you merged it in. It's annoying to have perfectly fine PR's lingering unmerged. The downside is that it's causing a proliferation of cucumber tags, but that can easily be cleaned up at a later date. |
Refactoring Revert and refactor unit tests Finish moving tests to separate modules Remove test feature files adding method to ABIResult (#342) * adding method to ABIResult Add method to get Method by name (#345) * adding util method to get a Method from an interface or contract by name string Change paths for tests Change makefile
I expect a common use case when preparing an ATC group will be to grab a Method from some parsed json file.
Rather than forcing the dev to maintain a separate list we should offer some utility method to get the Method by name.