-
-
Notifications
You must be signed in to change notification settings - Fork 24
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
Update more tests #250
Update more tests #250
Conversation
@@ -57,7 +59,7 @@ Execute (detects anagrams using case-insensitive possible matches): | |||
AssertEqual expected, FindAnagrams(candidates, subject) | |||
|
|||
Execute (does not detect a anagram if the original word is repeated): | |||
let candidates = ['go Go GO'] | |||
let candidates = ['goGoGO'] |
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.
This is not the same word repeated, but a single word with 3 unique letters. It was before this change a single word in three different cases.
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.
So you want me to revert? The reimplemented test comes from problem-specifications.
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.
No, then I would presume any problem with this would need to be tackled up there. If we have a word "go" and "we have three candidates that are three words "go" "Go" and "GO" then I would say those are not anagrams. But "go" and "goGoGO" would not be an anagram because anagrams have the same number of letters, not because this is two letters of the subject words, and they are repeated. Instead, it is because there are more characters in the proposed anagram than there is in the subject.
I could, of course be totally wrong here, as I am not an anagrammatist.
However, if you suspect that this is going to cause noise and more questions than it raises, we could also forego this test.
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 don’t know the original intention of the test, but I’m reading it similarly. The candidate should fail because of string length. That’s fairly clear, but a student might assume a candidate containing the subject is an anagram. So they’d fail the test.
You can suggest changes upstream, and we can make a follow-up PR if they’re accepted. There’s no rush to merge this in, but there’s also no compelling reason to sit on this. So I think it’d be simpler to just merge this now.
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.
Do you think that at some point, it was 3 words, as in a language with an array or list that looks like this: ['go', 'Go', 'GO']
and it was meant to be three separate words separated with a space, and as a string, rather than one string, one word? I want to make sure it is not a translation problem to a language that is using a string with spacing to differentiate words, rather than individual strings that would often denote words.
So I want to also make sure that perhaps we are not interpreting it (or if programmatically generated) incorrectly.
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.
If we're this uncertain, let's just not include the test since a student looking at the test suite for the first time will definitely be uncertain.
Co-authored-by: Victor Goff <[email protected]>
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.
Pre-approving with the suggestion to remove the <space><space>
before the last EOL character.
Edit: Sorry the spaces were not showing in the message due to being processed in the web UI.
Co-authored-by: Victor Goff <[email protected]>
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.
Looks good to me otherwise.
Co-authored-by: Victor Goff <[email protected]>
Related to #248