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

Throw Exception for wrong argtype in join method #2364

Merged
merged 12 commits into from
Oct 6, 2023

Conversation

Agent-Hellboy
Copy link
Contributor

No description provided.

Comment on lines +6671 to +6676
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);
}
Copy link
Collaborator

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.

Copy link
Contributor Author

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.

Copy link
Contributor Author

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.

Copy link
Collaborator

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.

Copy link
Collaborator

@Thirumalai-Shaktivel Thirumalai-Shaktivel Oct 6, 2023

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.

@Agent-Hellboy Agent-Hellboy marked this pull request as ready for review October 6, 2023 07:50
Agent-Hellboy and others added 3 commits October 6, 2023 14:11
@Shaikh-Ubaid
Copy link
Collaborator

please apply the above change and update the reference tests (./run_tests.py -u).

@Agent-Hellboy After the change, please update the reference tests like I mentioned above. Ideally do the following:

rm -rf ./tests/reference/*
./run_tests.py -u

# now just commit all the reference test changes

@Shaikh-Ubaid
Copy link
Collaborator

Shaikh-Ubaid commented Oct 6, 2023

@Agent-Hellboy I shared few comments above. This looks good to me overall. For further changes I created another issue at #2366.

Copy link
Collaborator

@Shaikh-Ubaid Shaikh-Ubaid left a 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.

@Shaikh-Ubaid Shaikh-Ubaid enabled auto-merge (squash) October 6, 2023 09:55
@Shaikh-Ubaid Shaikh-Ubaid merged commit 5c0b91e into lcompilers:main Oct 6, 2023
12 checks passed
Agent-Hellboy added a commit to Agent-Hellboy/lpython that referenced this pull request Oct 8, 2023
* 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>
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.

3 participants