Add join method in str by Agent-Hellboy · Pull Request #2354 · lcompilers/lpython

Agent-Hellboy · 2023-10-02T11:44:13Z

No description provided.

certik

I think this is great, thank you @Agent-Hellboy. I left a minor suggestion.

integration_tests/test_str_01.py

certik · 2023-10-02T11:49:31Z

You also need to update our reference tests using: ./run_tests -u and commit the changes.

OculusMode · 2023-10-02T11:54:35Z

src/runtime/lpython_builtin.py

+def _lpython_str_join(s:str, lis:list[str]) -> str:
+    res:str = ""
+    i:i32
+    for i in range(len(lis)):


Can be directly iterated on the list itself, this way it can be worked on iterables easily(in future). Every iterable can be called using next so I think that would be more better.

something like this can be more cleaner I think.

for ele in range(list_): res+=ele

If it works, we can use the simpler syntax. LPython doesn't yet work for user defined iterables, but eventually it will I think.

OculusMode · 2023-10-02T12:02:46Z

src/runtime/lpython_builtin.py

+    for i in range(len(lis)):
+        res+=s
+        res+=lis[i]
+    return res[1:]


thats not reliable. You are always assuming your str len will be 1 here, It should fail when str has len more than 1. You can prob use res[len(str):]

Good catch! I think even better would be something like:

if len(lis) == 0: return "" res = lis[0] for i in range(1, len(lis)): res += s + lis[i] return res

That works too, altho as long as there plans of iterables, part of len will probably be removed regardless.

Agent-Hellboy · 2023-10-02T15:12:53Z

You also need to update our reference tests using: ./run_tests -u and commit the changes.

okay, the CI tests are failing because of this, I didn't understand update our reference tests, what does run_tests -u do?

-u flag is not there in run_tests

    parser.add_argument("-j", "-n", "--no_of_threads", type=int,
                help="Parallel testing on given number of threads")
    parser.add_argument("-b", "--backends", nargs="*", default=["llvm", "cpython"],
                type=str, help="Test the requested backends (%s), default: llvm, cpython" % \
                        ", ".join(SUPPORTED_BACKENDS))
    parser.add_argument("-f", "--fast", action='store_true',
                help="Run supported tests with --fast")
    return parser.parse_args()

Agent-Hellboy · 2023-10-02T16:32:03Z

I guess, I need to parse over asr nodes of the list argument passed inside the join method to check if only str is passed or not
https://github.com/lcompilers/lpython/pull/2354/files#diff-147457a9117ab6d023c02f9a359d6060c248c4216cfcb47b1fafd17d885063feR6672
I am still exploring the codebase, I will try to do it in coming days

certik · 2023-10-02T18:41:34Z

It's the one in the root directory:

(lp) repos/lpython(main) $ ./run_tests.py --help
usage: run_tests.py [-h] [-u] [-l] [-t [TEST ...]] [-b [BACKEND ...]] [-v]
                    [--exclude-test [TEST ...]]
                    [--exclude-backend [BACKEND ...]] [--no-llvm]
                    [--skip-run-with-dbg] [-s] [--no-color]

LPython Test Suite

options:
  -h, --help            show this help message and exit
  -u, --update          update all reference results
...

certik · 2023-10-02T18:43:44Z

I guess, I need to parse over asr nodes of the list argument passed inside the join method to check if only str is passed or not

That's needed for error messages. But first let's get the two existing tests that you added (they both look great) to pass all CI. After CI passes, then we can either merge, or you can add some tests for errors.

src/lpython/semantics/python_ast_to_asr.cpp

Thirumalai-Shaktivel · 2023-10-03T05:34:24Z

integration_tests/test_str_01.py

    test_str_index()
    test_str_slice()
    test_str_repeat()
+    test_str_join()


Let's test test_str_join2

Thirumalai-Shaktivel

LGTM, Thank you!

Thirumalai-Shaktivel · 2023-10-03T05:36:12Z

integration_tests/test_str_01.py

+    a = "**"
+    p:list[str] = ["a","b"]
+    res:str = a.join(p)
+    assert res == "a**b"


We can test more cases like empty strings, more items in the list (more than 3), and so on...
This can be done in the subsequent PRs.

certik

I think this looks good, thanks!

ubaidsk · 2023-10-03T21:13:20Z

I just noticed this. @Agent-Hellboy this is amazing! I really appreciate it. Keep up the great work.

ubaidsk · 2023-10-06T06:19:30Z

src/lpython/semantics/python_ast_to_asr.cpp

+            ASR::call_arg_t passed_int;
+            passed_int.loc = loc;
+            passed_int.m_value = args[0].m_value;


I just noticed this. Here, I think passed_int should be named list_of_str (or something similar). (@Agent-Hellboy I remember we were experimenting with it by passing an integer.)

I think you can fix the above in your new PR #2364. You can just make the rename a separate commit.

Go ahead and create an issue, so that we don't forget.

created #2365

Add join method in str

8cf62af

certik approved these changes Oct 2, 2023

View reviewed changes

integration_tests/test_str_01.py Show resolved Hide resolved

OculusMode reviewed Oct 2, 2023

View reviewed changes

Add new test

baaf269

Agent-Hellboy marked this pull request as ready for review October 2, 2023 15:14

Add missing overload

9d523fc

Update test references

e7468a6

Thirumalai-Shaktivel reviewed Oct 3, 2023

View reviewed changes

src/lpython/semantics/python_ast_to_asr.cpp Outdated Show resolved Hide resolved

Remove unnecessary include statement

a4183b3

Thirumalai-Shaktivel reviewed Oct 3, 2023

View reviewed changes

Thirumalai-Shaktivel approved these changes Oct 3, 2023

View reviewed changes

Thirumalai-Shaktivel reviewed Oct 3, 2023

View reviewed changes

certik approved these changes Oct 3, 2023

View reviewed changes

certik merged commit 98e53f4 into lcompilers:main Oct 3, 2023

ubaidsk reviewed Oct 6, 2023

View reviewed changes

Comments

Conversation

Agent-Hellboy commented Oct 2, 2023

Uh oh!

certik left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

certik commented Oct 2, 2023

Uh oh!

OculusMode Oct 2, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Agent-Hellboy commented Oct 2, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Agent-Hellboy commented Oct 2, 2023

Uh oh!

certik commented Oct 2, 2023

Uh oh!

certik commented Oct 2, 2023

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Thirumalai-Shaktivel left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

certik left a comment

Choose a reason for hiding this comment

Uh oh!

ubaidsk commented Oct 3, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

OculusMode Oct 2, 2023 •

edited

Loading

Agent-Hellboy commented Oct 2, 2023 •

edited

Loading