-
-
Notifications
You must be signed in to change notification settings - Fork 110
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
enhance singular_noun #228
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We'll want to add some tests to capture the new expectation(s).
def handle_irregular_plural(word_to_check, inflection_result): | ||
if word_to_check in pl_sb_irregular.values(): | ||
for sing, plur in pl_sb_irregular.items(): | ||
if word_to_check in plur.split('|'): | ||
return sing | ||
return inflection_result if inflection_result is not False else word_to_check |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm guessing this function can be put in the global scope as a helper function (or as a classmethod or staticmethod). Better to do that to communicate that it's not depending on the containing scope.
[( | ||
handle_irregular_plural(cand, inflection_result) | ||
if ( | ||
not (count != 1 and cand.endswith('s')) or | ||
cand in pl_sb_irregular.values() | ||
) | ||
else cand | ||
), prep], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The solutions
expression was already near its limits for complexity. Let's see if we can't find a way to encapsulate some of this logic in a function or otherwise compartmentalize the logic here so it's not so complex.
trailer, | ||
) | ||
) | ||
for leader, (cand, prep), trailer in windowed_complete(word.split_, 2) | ||
if prep in pl_prep_list_da | ||
for inflection_result in [inflection(cand, count)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm a little worried that the call to inflection
has moved. I haven't yet absorbed why. Maybe it's correct, but I want to flag this as something to consider and explain before accepting it. Additional tests would give me more confidence in the choice.
>>> engine()._handle_long_compounds(Words("pair of scissors"), 2) | ||
'pairs of scissors' | ||
>>> engine()._handle_long_compounds(Words("men beyond hills"), 1) | ||
'man beyond hills' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The removal of these tests is concerning. These are here not only to document the expected behavior of this method, but also to serve to protect those behaviors. We should not remove these expectations without a good justification.
This fixes the issue with #222 and is an improvement. However, it is not a perfect solution for all other possible phrases.
Summary of Changes:
Added Handling for Irregular Plurals:
handle_irregular_plural()
, which checks if a word is an irregular plural and replaces it with its singular form when necessary.pl_sb_irregular.values()
to determine if the word is irregular and maps it back to its singular form.Improved Inflection Handling:
count != 1
but the word already ends in 's'.handle_irregular_plural()
.Refactored Inflection Logic:
inflection(cand, count)
call is now stored in afor
loop to ensure that it is only computed once and can be used in multiple checks.cand
) should be replaced with the inflected version or left unchanged.Removed Docstring Examples:
doctests
), which were present in the original method.Possible Future Improvements
Here is a script that illustrates other scenarios that are related conceptually but that this PR decided to not address at this time, for your future improvements:
TEST SCRIPT