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

feat: runtime type checker created #204

Closed
wants to merge 4 commits into from

Conversation

Niraj-Kamdar
Copy link

@Niraj-Kamdar Niraj-Kamdar commented Jun 17, 2021

relates: #179

Same method can be used for Transaction class to perform typecheck
Let me know if you like the solution and would want me to make the other classes type safe.

@CLAassistant
Copy link

CLAassistant commented Jun 17, 2021

CLA assistant check
All committers have signed the CLA.

@Niraj-Kamdar
Copy link
Author

Let me know which formatter are you guys using. I have used my default one for now.

@Niraj-Kamdar
Copy link
Author

Unittests are working locally but I think there seems to be errors related to invalid types in integration tests. It seems like I have to go through CI log and change relevant test suite.

@Niraj-Kamdar
Copy link
Author

There is bug in testing scripts:

FAILURE in step 'I build an application transaction with operation "create", application-id 0, sender "BH55E5RMBD4GYWXGX5W5PJ5JAHPGM5OXKDQH5DC4O2MGI7NW4H6VOE4CP4", approval-program "programs/loccheck.teal.tok", clear-program "programs/one.teal.tok", global-bytes 1, global-ints 0, local-bytes 1, local-ints 0, app-args "str:test", foreign-apps "5555,6666", foreign-assets "", app-accounts "", fee 1234, first-valid 9000, last-valid 9010, genesis-hash "Mf0h6zjkEIEZPtNM3zsrg+iHQFS0fZxhgr7w35I464M=", extra-pages 0':
  Feature:  Transaction fee test
  Scenario: App Call Fee Test -- @1.1 
Assertion Failed: expected type of approval_program: <class 'bytes'> but got <class 'bytearray'>
exception: expected type of approval_program: <class 'bytes'> but got <class 'bytearray'>

As mentioned in log approval_program should be bytes type but passed bytearray instead. It can be solved by typecasting to bytes but I don't know how to do it in .feature files.

An alternate solution can be auto typecasting in bytes

@michielmulders
Copy link

@jasonpaulos Can you help here? Thanks!

@Niraj-Kamdar
Copy link
Author

Any updates?

Copy link
Contributor

@jasonpaulos jasonpaulos left a comment

Choose a reason for hiding this comment

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

Hi @Niraj-Kamdar, I apologize for the delay in reviewing. I believe we would be interested in this change if you could make TypeCheck into a function decorator instead of a base class, since then we could type check more than just __init__ methods.

Additionally, could you refrain from running a formatter in this PR? Soon we will add a formatter to this repo and reformat all the code, but that will happen in a separate PR.

@Niraj-Kamdar
Copy link
Author

Hi @Niraj-Kamdar, I apologize for the delay in reviewing. I believe we would be interested in this change if you could make TypeCheck into a function decorator instead of a base class, since then we could type check more than just __init__ methods.

Having it as a class has its own benefits. This will work for nested inheritance because of MRO and I think it's better than decorating every classes out there.

I can create a friendly decorator though if you want to type check a particular function.

@jannotti
Copy link
Contributor

jannotti commented Jan 22, 2024

Is this technique usable when the argument types or not simple int, str, etc? Does isinstance(x, Union[int,str]) work?

Edit: It does. So I would prefer we avoid the change that made strs stop working in some of the constructors that accept a string or an int for an int parameter.

@jasonpaulos
Copy link
Contributor

Closing due to inactivity

@jasonpaulos jasonpaulos closed this Jun 5, 2024
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.

5 participants