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

Added str.replace() #2587

Merged
merged 9 commits into from
Mar 10, 2024
Merged

Added str.replace() #2587

merged 9 commits into from
Mar 10, 2024

Conversation

Kishan-Ved
Copy link
Contributor

Towards: #2356

In this PR, I have implemented str.replace() completely. Relevant tests have also been added.

Parameters:

  • old – old substring you want to replace.
  • new – new substring which would replace the old substring.
  • count – (Optional ) the number of times you want to replace the old substring with the new substring.

@Kishan-Ved
Copy link
Contributor Author

I don't get any errors upon running ./run_tests.py locally. However, here, 2 checks fail due to this. Please let me know the fix for this. @Thirumalai-Shaktivel @Shaikh-Ubaid

@Thirumalai-Shaktivel
Copy link
Collaborator

It's the C test that fails, you can reproduce it using the following command:

$ cd integration_tests
$ ./run_tests.py -b c

@Thirumalai-Shaktivel
Copy link
Collaborator

Please mark this PR ready for review once it is ready!

@Thirumalai-Shaktivel Thirumalai-Shaktivel marked this pull request as draft March 9, 2024 05:44
@Kishan-Ved Kishan-Ved marked this pull request as ready for review March 9, 2024 09:12
@Kishan-Ved
Copy link
Contributor Author

It's the C test that fails, you can reproduce it using the following command:

$ cd integration_tests
$ ./run_tests.py -b c

Fixed. Thank you @Thirumalai-Shaktivel .

@Kishan-Ved
Copy link
Contributor Author

@Shaikh-Ubaid @Thirumalai-Shaktivel Please let me know if this is good to be merged.

@Shaikh-Ubaid
Copy link
Collaborator

I think the implementation fails for the following example:

% cat examples/expr2.py   
from lpython import i32

def main0():
    x: str
    x = "abc"
    print(x.replace("", ","))
    assert x.replace("", ",") == ",a,b,c,"

main0()
% python examples/expr2.py
,a,b,c,
% lpython examples/expr2.py
,a,b,c
AssertionError

@Kishan-Ved
Copy link
Contributor Author

I think the implementation fails for the following example:

% cat examples/expr2.py   
from lpython import i32

def main0():
    x: str
    x = "abc"
    print(x.replace("", ","))
    assert x.replace("", ",") == ",a,b,c,"

main0()
% python examples/expr2.py
,a,b,c,
% lpython examples/expr2.py
,a,b,c
AssertionError

Fixed. Thanks.

@@ -0,0 +1,13 @@
(KEYWORD "def") 0:2
Copy link
Collaborator

Choose a reason for hiding this comment

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

How did this file got commited? I think it should not be present.

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 have not committed this, as this is in stdout, so I believe it is due to updated references.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I have not committed this, as this is in stdout, so I believe it is due to updated references.

Can you trying doing:

rm -rf tests/reference/* (then press 'y' if asked for confirmation)
./run_tests.py -u
git add tests
git commit -m "TEST: Update reference tests"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What is supposed to happen? I did this, but nothing got committed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Upon rerunning these commands, I noticed there are no net changes. All the deleted files reappear.

Copy link
Collaborator

@Shaikh-Ubaid Shaikh-Ubaid Mar 10, 2024

Choose a reason for hiding this comment

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

Did you follow the exact steps in #2587 (comment)?

The output shared in #2587 (comment) seems strange and does not look like exact steps were used.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Upon rerunning these commands, I noticed there are no net changes. All the deleted files reappear.

Can you rebase over the latest main? I think that might help remove the stray file.

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 believe this has been done from your side. Thanks.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think the file is now removed. We had to rebase over latest main, rebuild the project and then follow the steps in #2587 (comment).

Copy link
Contributor Author

@Kishan-Ved Kishan-Ved Mar 10, 2024

Choose a reason for hiding this comment

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

Ok, thanks. I'm deleting the output now, as it is too long.

assert a.replace("b","k",1) == "zzaaakracadabra"
assert a.replace("b","k",2) == "zzaaakracadakra"
assert a.replace("zza","yo",2) == "yoaabracadabra"
assert x.replace("", ",") == ",a,b,c,"
Copy link
Collaborator

Choose a reason for hiding this comment

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

We next need support for compile-time strings in replace. For example,

% cat examples/expr2.py 
def main0():
    print("abc".replace("", ","))

main0()
% python examples/expr2.py 
,a,b,c,
% lpython examples/expr2.py
semantic error: 'str' object has no attribute 'replace'
 --> examples/expr2.py:2:11
  |
2 |     print("abc".replace("", ","))
  |           ^^^^^^^^^^^^^^^^^^^^^^ 


Note: Please report unclear or confusing messages as bugs at
https://github.com/lcompilers/lpython/issues.

I think this can be tackled in a separate PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure. Please let me know how to implement this. Also, is this done for all other functions?

Copy link
Collaborator

@Shaikh-Ubaid Shaikh-Ubaid Mar 10, 2024

Choose a reason for hiding this comment

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

Search for "upper" in python_ast_to_asr.cpp and you will find the place(s) for compile-time implementation.

I would suggest supporting it in a separate PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, thanks.

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.

This looks good to me. Thanks for working on this! Great work! I appreciate it.

@Shaikh-Ubaid Shaikh-Ubaid enabled auto-merge (squash) March 10, 2024 18:59
@Shaikh-Ubaid Shaikh-Ubaid merged commit 3e6ddc6 into lcompilers:main Mar 10, 2024
13 checks passed
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