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

Reorder _validate_parameter_type to avoid isclass check #312

Conversation

michaeldiamant
Copy link
Contributor

Reorders checks in Reorder _validate_parameter_type to avoid isclass check.

Rationale: Since there was some discussion on the topic in #256 (comment), I paired with @tzaffi to better understand the runtime type system checks. While pairing, we observed that it's possible to remove a check and retain equivalent functionality provided we're OK with a different granularity error message.

Treat as optional. I'll happily close if you'd prefer to keep as is.

raise TealInputError(
f"Function has parameter {parameter_name} of declared type {ptype} which is not a class"
)

if ptype in (Expr, ScratchVar):
Copy link
Contributor

Choose a reason for hiding this comment

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

FYI - my merges are about to repeat this work.

And optionally: Since you are returning out of the if's you can remove the else's and write the last 2 conditions as:

            if SubroutineDefinition._is_abi_annotation(ptype):
                return abi.type_spec_from_annotation(ptype)
            raise TealInputError(
                f"Function has parameter {parameter_name} of disallowed type {ptype}. "
                f"Only the types {(Expr, ScratchVar, 'ABI')} are allowed"
            )

@michaeldiamant
Copy link
Contributor Author

Closing - #310 brings the same change in.

@michaeldiamant michaeldiamant deleted the abi-subroutine_subroutinedefinition_reorder_checks branch May 2, 2022 14:14
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.

2 participants