easyPythonpi icon indicating copy to clipboard operation
easyPythonpi copied to clipboard

Defects in is_palindrome() and remove_punctuation()

Open AndrewHUNGNguyen opened this issue 2 years ago • 6 comments

is_palindrome() fails test cases where the string is a sentence with the same characters.

remove_punctuation() fails on unicode punctuations like © and ™. We may need to include unicode characters as punctuations to remove as well listed at https://www.compart.com/en/unicode/category/Po

@extinctsion let me know your thoughts on punctuations we need to include.

AndrewHUNGNguyen avatar Oct 21 '23 20:10 AndrewHUNGNguyen

@extinctsion assign this issue to me.

AndrewHUNGNguyen avatar Oct 21 '23 20:10 AndrewHUNGNguyen

I think it's practically impossible to include all unicode punctuations as one may create custom using their own unicode. Hence, it would be better to only include those punctuations which are frequently used.

extinctsion avatar Oct 22 '23 09:10 extinctsion

I think it's practically impossible to include all unicode punctuations as one may create custom using their own unicode. Hence, it would be better to only include those punctuations which are frequently used.

That's true. I just thought of this to think of the long term impact of this method. Rather than only considering most commonly used punctuation marks, how about we modify the parameters of the method in the sense that we let users of this method to remove a punctuation marks or list of punctuation marks of their choice?

If we should keep remove_punctuation as is, then we will stick with just resolving the defect in is_palindrome().

@extinctsion

AndrewHUNGNguyen avatar Oct 23 '23 19:10 AndrewHUNGNguyen

@extinctsion in case you want to modify is_palindrome only, I created a PR at #75

AndrewHUNGNguyen avatar Oct 23 '23 20:10 AndrewHUNGNguyen

@extinctsion I created a PR at #76

AndrewHUNGNguyen avatar Oct 27 '23 18:10 AndrewHUNGNguyen

Thanks. Merged it in the main branch. Now if you will raise any PR, it will test for all the test cases and then it will be approved for merge.

extinctsion avatar Oct 27 '23 19:10 extinctsion

issue resolved

extinctsion avatar Oct 26 '24 17:10 extinctsion