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.

Sign up for GitHub

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

Jump to bottom

Add isalpha, istitle, and title APIs in str #2357

Merged
merged 4 commits into from
Oct 9, 2023
Merged

Add isalpha, istitle, and title APIs in str #2357

merged 4 commits into from
Oct 9, 2023

Conversation

Agent-Hellboy
Copy link
Contributor

No description provided.

@certik
Copy link
Contributor

certik commented Oct 2, 2023

@Agent-Hellboy thanks, looks good. Make sure to update reference tests using ./run_tests -u.

Thirumalai-Shaktivel
Thirumalai-Shaktivel reviewed Oct 3, 2023
View reviewed changes
src/runtime/lpython_builtin.py Outdated
@@ -764,6 +764,72 @@ def _lpython_str_upper(x: str) -> str:
return res


@overload
def _lpython_str_isalpha(s: str) -> bool:
char: str
Copy link
Collaborator

Choose a reason for hiding this comment

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

Here is the cause for the CI failure: #2359

I think renaming it or fixing the bug would resolve the CI failure.

@Thirumalai-Shaktivel
Copy link
Collaborator

Otherwise, this PR looks great!

@Thirumalai-Shaktivel
Copy link
Collaborator

@Agent-Hellboy you can try cd integration_tests && ./run_tests.py -b c to test c backend locally.

@Agent-Hellboy
Copy link
Contributor Author

@Agent-Hellboy you can try cd integration_tests && ./run_tests.py -b c to test c backend locally.

okay , sure

@Thirumalai-Shaktivel
Copy link
Collaborator

Thirumalai-Shaktivel commented Oct 3, 2023 •
edited
Loading

I'm not sure what's causing the CI failure. Try debugging it logically and create an issue for it.
You can use lpython a.py --show-c to view the c code and lpython a.py --backend=c to compile LPython using the C backend.

@Agent-Hellboy
Copy link
Contributor Author

I'm not sure what's causing the CI failure. Try debugging it logically and create an issue for it. You can use lpython a.py --show-c to view the c code and lpython a.py --backend=c to compile LPython using the C backend.

sure, I will try to do it today

@Agent-Hellboy
Copy link
Contributor Author

@Thirumalai-Shaktivel, how can I link libasr library I generated main.c which has included some header file form libasr, but i don't know how to link libasr to compile it with gcc.

@Thirumalai-Shaktivel
Copy link
Collaborator

Can you please share some details of where you are trying to link libasr?

@Thirumalai-Shaktivel
Copy link
Collaborator

Thirumalai-Shaktivel commented Oct 8, 2023 •
edited
Loading

If you are trying to compile C code generated using --show-c, try specifying --backend=c in lpython. It links all the libraries.
Like, lpython a.py --backend=c will compile the generated c code using gcc or clang.

@Agent-Hellboy
Copy link
Contributor Author

If you are trying to compile C code generated using --show-c, try specifying --backend=c in lpython. It links all the libraries.

okay

@certik certik marked this pull request as draft October 8, 2023 06:52
@certik
Copy link
Contributor

certik commented Oct 8, 2023

Once it is ready for review, go ahead and click on the "Ready for review" button.

@Agent-Hellboy Agent-Hellboy mentioned this pull request Oct 8, 2023
Open
@Agent-Hellboy Agent-Hellboy marked this pull request as ready for review October 8, 2023 10:10
certik
certik reviewed Oct 8, 2023
View reviewed changes
src/runtime/lpython_builtin.py Outdated
@@ -685,6 +685,71 @@ def _lpython_str_join(s:str, lis:list[str]) -> str:
res += s + lis[i]
return res

@overload
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do these need @overload? I think they are not overloaded, are they?

certik
certik approved these changes Oct 8, 2023
View reviewed changes
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 fine. I left one minor comment.

@Thirumalai-Shaktivel would you mind please reviewing this as well, to make sure I didn't miss anything? If it looks good, you can merge it.

Thirumalai-Shaktivel
Thirumalai-Shaktivel reviewed Oct 8, 2023
View reviewed changes
integration_tests/test_str_01.py
res: bool = a.istitle()
res2: bool = b.istitle()
assert res == True
assert res2 == False
Copy link
Collaborator

@Thirumalai-Shaktivel Thirumalai-Shaktivel Oct 8, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Add test for empty string.
Also add " Hello", "HeLlo" or something similar

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, this was an edge case
CPython istitle and title is not clear in documentation
updated the code according to the response from cpython

>>> l="\tHello"
>>> l.istitle()
True
>>> l="\thello"
>>> l.istitle()
False

@Thirumalai-Shaktivel
Copy link
Collaborator

Rest looks great, I left minor improvements.
Also, we have to resolve the conflicts.

Thirumalai-Shaktivel
Thirumalai-Shaktivel reviewed Oct 9, 2023
View reviewed changes
src/runtime/lpython_builtin.py Outdated
Comment on lines 735 to 772
<<<<<<< HEAD
<<<<<<< HEAD
elif ch.isalpha() and (ord('A') <= ord(ch) and ord(ch) <= ord('Z')):
=======
elif ch.isalpha() and ch.isupper():
>>>>>>> 5c5bcc620 (Fix merge conflict)
=======
elif ch.isalpha() and (ord('A') <= ord(ch) and ord(ch) <= ord('Z')):
>>>>>>> f7fcb9835 (Fix test references)
if word_start:
word_start = False
else:
return False # Found an uppercase character in the middle of a word
<<<<<<< HEAD
<<<<<<< HEAD
elif ch.isalpha() and (ord('a') <= ord(ch) and ord(ch) <= ord('z')):
=======
elif ch.isalpha() and ch.islower():
>>>>>>> 5c5bcc620 (Fix merge conflict)
=======
elif ch.isalpha() and (ord('a') <= ord(ch) and ord(ch) <= ord('z')):
>>>>>>> f7fcb9835 (Fix test references)
if word_start:
return False # Found a lowercase character in the middle of a word
word_start = False
else:
word_start = True

return True


<<<<<<< HEAD
<<<<<<< HEAD
=======

>>>>>>> 5c5bcc620 (Fix merge conflict)
=======
>>>>>>> f7fcb9835 (Fix test references)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Remove merge conflict tags

Suggested change
<<<<<<< HEAD
<<<<<<< HEAD
elif ch.isalpha() and (ord('A') <= ord(ch) and ord(ch) <= ord('Z')):
=======
elif ch.isalpha() and ch.isupper():
>>>>>>> 5c5bcc620 (Fix merge conflict)
=======
elif ch.isalpha() and (ord('A') <= ord(ch) and ord(ch) <= ord('Z')):
>>>>>>> f7fcb9835 (Fix test references)
if word_start:
word_start = False
else:
return False # Found an uppercase character in the middle of a word
<<<<<<< HEAD
<<<<<<< HEAD
elif ch.isalpha() and (ord('a') <= ord(ch) and ord(ch) <= ord('z')):
=======
elif ch.isalpha() and ch.islower():
>>>>>>> 5c5bcc620 (Fix merge conflict)
=======
elif ch.isalpha() and (ord('a') <= ord(ch) and ord(ch) <= ord('z')):
>>>>>>> f7fcb9835 (Fix test references)
if word_start:
return False # Found a lowercase character in the middle of a word
word_start = False
else:
word_start = True
return True
<<<<<<< HEAD
<<<<<<< HEAD
=======
>>>>>>> 5c5bcc620 (Fix merge conflict)
=======
>>>>>>> f7fcb9835 (Fix test references)
elif ch.isalpha() and (ord('A') <= ord(ch) and ord(ch) <= ord('Z')):
if word_start:
word_start = False
else:
return False # Found an uppercase character in the middle of a word
elif ch.isalpha() and (ord('a') <= ord(ch) and ord(ch) <= ord('z')):
if word_start:
return False # Found a lowercase character in the middle of a word
word_start = False
else:
word_start = True
return True

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes

@Agent-Hellboy
Copy link
Contributor Author

Agent-Hellboy commented Oct 9, 2023 •
edited
Loading

merge it for now, from now on, I will pick test cases from CPython
I have tested everything from https://github.com/Agent-Hellboy/cpython/blob/main/Lib/test/test_str.py#L696
https://github.com/Agent-Hellboy/cpython/blob/main/Lib/test/test_str.py#L734

@Agent-Hellboy @Thirumalai-Shaktivel
Update integration_tests/test_str_01.py
b070bd0 
Co-authored-by: Thirumalai Shaktivel <74826228+Thirumalai-Shaktivel@users.noreply.github.com>
@certik certik enabled auto-merge (squash) October 9, 2023 07:03
@certik certik merged commit ac65227 into lcompilers:main Oct 9, 2023
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Thirumalai-Shaktivel Thirumalai-Shaktivel Thirumalai-Shaktivel left review comments

@certik certik certik approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Agent-Hellboy @certik @Thirumalai-Shaktivel