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

Add join method in str #2354

Merged
merged 5 commits into from
Oct 3, 2023
Merged

Conversation

Agent-Hellboy
Copy link
Contributor

No description provided.

Copy link
Contributor

@certik certik left a comment

Choose a reason for hiding this comment

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

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

integration_tests/test_str_01.py Show resolved Hide resolved
@certik
Copy link
Contributor

certik commented Oct 2, 2023

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

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

@OculusMode OculusMode Oct 2, 2023

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

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

Choose a reason for hiding this comment

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

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):]

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

@Agent-Hellboy
Copy link
Contributor Author

Agent-Hellboy commented Oct 2, 2023

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 Agent-Hellboy marked this pull request as ready for review October 2, 2023 15:14
@Agent-Hellboy
Copy link
Contributor Author

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
Copy link
Contributor

certik commented Oct 2, 2023

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
Copy link
Contributor

certik commented Oct 2, 2023

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.

@@ -55,6 +70,7 @@ def check():
test_str_index()
test_str_slice()
test_str_repeat()
test_str_join()
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's test test_str_join2

Copy link
Collaborator

@Thirumalai-Shaktivel Thirumalai-Shaktivel left a comment

Choose a reason for hiding this comment

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

LGTM, Thank you!

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

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@certik certik left a comment

Choose a reason for hiding this comment

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

I think this looks good, thanks!

@certik certik merged commit 98e53f4 into lcompilers:main Oct 3, 2023
@Shaikh-Ubaid
Copy link
Collaborator

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

Comment on lines +6676 to +6678
ASR::call_arg_t passed_int;
passed_int.loc = loc;
passed_int.m_value = args[0].m_value;
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

created #2365

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