search

extinctsion / easyPythonpi

A python library for the beginners. The program includes simple calulations. Genuine PRs will be merged without delays
https://pypi.org/project/easyPythonpi/
14 stars 36 forks source link

Defects in is_palindrome() and remove_punctuation() #68

Open AndrewHUNGNguyen opened 9 months ago

AndrewHUNGNguyen commented 9 months ago

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 commented 9 months ago

@extinctsion assign this issue to me.

extinctsion commented 9 months ago

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.

AndrewHUNGNguyen commented 9 months ago

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 commented 9 months ago

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

AndrewHUNGNguyen commented 8 months ago

@extinctsion I created a PR at #76

extinctsion commented 8 months ago

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.