-
-
Notifications
You must be signed in to change notification settings - Fork 550
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
pangram: Slim down descriptions to generate better test identifiers #1533
Conversation
Banal description adds unnecessary length to test method identifiers.
I had split this into two commits to make it easier to separate review nesting and description changes. |
btw, The strategy I use to create slim descriptions is imagine the word "Test" preceding the description (because including it would be redundant) |
Nice find. TIL something, and it is only the morning. |
Co-Authored-By: Ryan Potts <[email protected]>
@rpottsoh I went to push a similar change to your suggestion into |
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.
Excellent changes again. Left a small comment.
"expected": false | ||
}, | ||
{ | ||
"description": "perfect lower case", |
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 was wondering if we should maybe rephrase this test case or add something to the README to describe what a perfect pangram is.
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.
That sounds like a good idea. (different PR though for the README). "perfect pangram in lower case"
might be a better description of the 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.
Context being the pangram, which is why I think that the wording ended up being the way it became.
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 am in favour of adding to the readme as wel.
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.
@kotp @SleeplessByte If we do a separate PR for the readme, is this PR ok to be merged?
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.
Yes, I think so.
Another one merged @bencoman 🚀 |
thx all. |
Addresses exercism/problem-specifications#1533 This commit changes the test descriptions so that they no longer conform to the canonical test descriptions. Instead, they are modified such that they read "pangram ...", e.g. "pangram with empty sentence". This helps readability.
The purpose of the "description" field is to derive identifiers for naming generated test methods, but some descriptions have tended to verbosity ending up with long identifiers (up to 150 characters). Such long method names are harder for students to work with and also are awkward for some IDE GUIs.
Issue #1473 proposed adding a separate "identifier" field, but I was required to first try improving "descriptions" to better suit "identifier" generation. This PR aims to slim down the generated identifiers without losing substantive information about the test.
To start with, "Check if the given string is a pangram" is a rather banal redundant description unworthy of nesting.