-
Notifications
You must be signed in to change notification settings - Fork 165
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
Throw Exception for wrong argtype in join method #2364
Throw Exception for wrong argtype in join method #2364
Conversation
ASR::expr_t *arg_sub = args[0].m_value; | ||
ASR::ttype_t *arg_sub_type = ASRUtils::expr_type(arg_sub); | ||
if(!ASR::is_a<ASR::List_t>(*arg_sub_type)){ | ||
throw SemanticError("str.join() takes type list only", | ||
loc); | ||
} |
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 change seems good. You can add an error reference test for this. Here is an example of how to add an error reference test 7c2bbac. You just add a test file in tests/errors
. In the test file, you pass a type apart from list to str.join()
. You enable this test by adding it to tests/tests.toml
. Then you just need to update the reference tests using ./run_tests.py -u
. This will add a new reference .stderr
and .json
files.
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.
Actually, I am still not checking the type of element inside the list which should be str, I checked grammar for asr but didn't get anything on how to iterate over list expression as it must be a list of str expressions. ast is getting generated perfectly but, I don't know how can I map ast nodes to asr nodes.
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 did exact same thing you suggested, but CI is still failing , I must have missed something.
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.
Try updating refs using ./run_tests.py -u
and stage all and commit the updated refs.
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.
After implementing and committing the required changes, you can do the following to test everything on your machine:
$ ./build0.sh
$ ./build1.sh
$ ./run_tests.py # if this fails, check for the failure and do ./run_tests.py -u to update refs.
$ cd integration_tests && ./run_tests.py # Here you can do `./run_tests.py -b c` to test c backend, use `-h` option to view all the available options
If everything works, then you are good to go.
Co-authored-by: Shaikh Ubaid <shaikhubaid769@gmail.com>
@Agent-Hellboy After the change, please update the reference tests like I mentioned above. Ideally do the following:
|
@Agent-Hellboy I shared few comments above. This looks good to me overall. For further changes I created another issue at #2366. |
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.
Amazing work @Agent-Hellboy ! Thank you so much for this! This looks good to me.
* Throw Exception for wrong argtype in join method * Remove SemanticError from test_str_01.py * Add reference for test handling join method * Remove .stdout file * Run reference test * Remove unnecessary files * Include update references * Update tests/errors/string_01.py Co-authored-by: Shaikh Ubaid <shaikhubaid769@gmail.com> * Update test reference * Remove .stdout file * Add test for corner cases * Remove unused import --------- Co-authored-by: Shaikh Ubaid <shaikhubaid769@gmail.com>
No description provided.