Lowercase function with test cases for thofstrand#186
Lowercase function with test cases for thofstrand#186thofstrand wants to merge 2 commits intokscanne:masterfrom
Conversation
| else: | ||
| return word | ||
|
|
||
| def combineToString(char_list): |
There was a problem hiding this comment.
I see great use of inbuilt functions, but this combineToDefined defined functions could be used in a single line to reduce extra code.
There was a problem hiding this comment.
This might be wise but I also feel as though the main function is already way too large. I think splitting it actually makes for fewer lines of code in the long run.
S23/thofstrand/lowerCase.py
Outdated
| lowCase = word.lower() | ||
| caps = ['A','E','I','O','U','Á','É','Í','Ó','Ú'] | ||
| if word[0] == 'n' or word[0] == 't': | ||
| if word[1] in caps and word[2].isalpha(): #must check isalpha as Õ parses as `O`,`~` |
There was a problem hiding this comment.
I quiet didn't understand the 3 letter must be a alpha or not. I think just the 1 letter must be a n or a t and the second letter in caps.
There was a problem hiding this comment.
Essentially one of the chars ( Õ) that was an input splits into two characters (O,~) when I am separating the string by characters. I used isalpha to check for this and make sure i am not counting that character as one of the listed ones. I do see another issue as I am not checking the length of the word, so this I can fix!
S23/thofstrand/lowerCase.py
Outdated
|
|
||
| def lowerCase(word, lang): | ||
| word = word.strip() | ||
| langStart = lang[:2] |
There was a problem hiding this comment.
We can add a condition if the user is giving false/errorful language as input.
There was a problem hiding this comment.
Good thought! I am adding this now!
|
The code was quite understandable, but some lines could be changed/improved. I loved the use of functions in the code. Just some more test cases are missed. Overall, it was a good piece of code, I got to learn something. |
|
I am fine with the changes now. Signing this off :) |
kscanne
left a comment
kscanne
left a comment
There was a problem hiding this comment.
Some additional comments I would have made on an initial review.
| lang = lang.split('-') | ||
| langStart = lang[0] | ||
|
|
||
| if len(langStart) != 2: |
There was a problem hiding this comment.
You should look at the BCP-47 spec. There are (many) language codes that have length 3.
| langStart = lang[0] | ||
|
|
||
| if len(langStart) != 2: | ||
| return "Invalid Language" |
There was a problem hiding this comment.
This is a place to raise an exception, not return a string error message.
| if len(langStart) != 2: | ||
| return "Invalid Language" | ||
|
|
||
| if "en" == langStart: |
There was a problem hiding this comment.
Why just English? Shouldn't this be the default for many languages that have case?
|
|
||
| if "I" in word: | ||
| word_list = [*word] | ||
| indices = [i for i, x in enumerate(word_list) if x == "I"] |
There was a problem hiding this comment.
Simpler to use the string "replace" method vs. converting to list and back?
| caps = ['A','E','I','O','U','Á','É','Í','Ó','Ú'] | ||
| if word[0] == 'n' or word[0] == 't': | ||
| if len(word) > 2: | ||
| if word[1] in caps and word[2].isalpha(): #must check isalpha as Õ parses as `O`,`~` |
There was a problem hiding this comment.
There's a cleaner way to handle this; will discuss in class.
| lower_word_list[-1] = 'ς' | ||
| lower_word = combineToString(lower_word_list) | ||
| return lower_word | ||
| else: |
There was a problem hiding this comment.
You should have many more test cases for other languages like Spanish, French, etc. that are handled like English. These tests will fail given the current code.
3 participants
Created function that lower cases a variety of languages based on input. Includes many test cases. Just run the .py file in order to run the tests.