[Darts] Slim down generated method names

[bencoman]

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 the consensus there was to first try improving "descriptions" to better suit "identifier" generation. This PR attempts to slim down the generated identifiers without losing substantive information about the test.

For the longest descriptions here, I couldn't recognize the significance of...

dart whose coordinates sum to > 1 but whose radius to origin is <= 1

While considering a shorter alternative, it helped consistency to also rearranged the shorter descriptions to be more explicit about each test condition, and add a couple of tests each side of boundary conditions.

[rpottsoh]

I'd like to remove the word dart from all the descriptions. The phrasing could be improved (commas and phrase ordering improved) by doing away with that single word. Unless we decide to start throwing other things at the dart board.... Thoughts?

Outer regions, dart on border
becomes
On border of outer region

[bencoman]

I'd like to remove the word dart from all the descriptions

I agree entirely, just thought others might believe that a step too far. I'll wait for one more review before doing it.

[ErikSchierboom]

Some minor nits. And I'm absolutely in favor of removing the "dart" bit from the descriptions. That is just superfluous in this context. Thanks again for working on this!

[bencoman]

I was finding the sub-word repetition of "inside inner-middle" & "outside middle-outer" slightly brain bending. Had an inspiration to substitute "within" & "beyond". Not strongly tied to it, just sharing for your thoughts.

P.S. I just noticed that for integrating suggestions the "Files changed" tab has an extra button "Add to batch" that is missing from the "Conversations" tab. So I'll use that next time.

[rpottsoh]

See if you can homogenize the descriptions. What I mean is that some refer to a region and others a border. Is kind of confusing. Sometime there is hyphenation other times not. However you decide best to "unbend your brain" oO works for me. 😄

[bencoman]

Removed last commit. I had bumped the major version on the wrong PR.

[petertseng]

that is interesting. I see that asymmetry (not being on the lines x = y or x = -y) is coincidentally a trait of the cases added in #1488, but I don't think it is necessary for the thing being tested, with the following example: x = 3.5, y = 3.5

• is on x = y
• sums to > 5
• radius is <= 5

Therefore, asymmetry is not the actual thing being tested in these test cases, and writing their description as asymmetric loses information as to why this test case is here. Some possible choices I see here are:

• Decide that losing this information from the description is okay, with the justification being that it is possible to obtain it by reviewing commit history.
• Find some way to write this in the description that is still acceptable. Since I am raising this point it behooves me to give some suggestions... unfortunately some of my suggestions require some form of jargon
  • Within inner circle by Euclidean distance, not Manhattan
  • Within inner circle by Euclidean distance, not taxicab
  • Within inner circle by radius, not addition
  • Within inner circle but not its inscribed square
• Maybe this is a situation where nesting these three cases is appropriate, under a cases whose description describes what's in common between these three test cases.

[bencoman]

I liked all four suggestions. My preference would be the third as the least exotic, closely followed by the first as well defined terms.

However, thanks @petertseng for the background on the purpose of those #1488 tests. The existing description "A dart whose coordinates sum to > 1 but whose radius to origin is <= 1 is scored in the inner circle" didn't impart to me that it was to prevent "an erroneous-but-passing solution where the sum of the absolute values of the coordinates of the dart are compared to the radius thresholds". This is a good example where the "what" didn't impart the "why" to me, and your third suggestion better describes "why".

Now considering the three test pairs I arranged near either side of each boundary, e.g...

    {
      "property": "score",
      "description": "Just within middle circle",
      "input": {
        "x": -3.5,
        "y": 3.5
      },
      "expected": 5
    },   
    {
      "property": "score",
      "description": "Just beyond middle circle",
      "input": {
        "x": -3.6,
        "y": -3.6
      },
      "expected": 1
    },     

simple addition won't be able to satisfy all three pairs.

So it seems I inadvertently made those #1488 tests redundant.
They might either be removed or re-purposed for asymmetry as I've done.

[petertseng]

I checked and you are right that the "just within" tests all will detect an implementation that tests absolute addition:

== anti-verify abs score ==
5. Just within inner circle: Got 5 instead of 10
7. Just within middle circle: Got 1 instead of 5
9. Just within outer circle: Got 0 instead of 1
11. Asymmetric position within inner circle: Got 5 instead of 10
12. Asymmetric position between inner and middle circles: Got 1 instead of 5
13. Asymmetric position between middle and outer circles: Got 0 instead of 1

So you are correct that these asymmetric points are all redundant.

I would say the answer for "should these tests be kept?" would depend on whether it is useful (would find any mistaken implementations) to have any points that aren't on x = y and x = -y. I wasn't sure if there are any, so I can be convinced either way. Though note that there are already some earlier test cases that show that.

I think it perhaps makes the most sense to remove these three tests then? Should the other three tests' descriptions be changed from "just within" to something else resembling the above suggestions? Or is "just within" enough information to figure it out?

I think I'm good trusting your judgment either way at this point

[bencoman]

I can't imagine anyone really doing it, but theoretically without an asymmetric test students might

get away with... sqrt(2*x*x) 
rather than... sqrt(x*x + y*y) 

But one asymmetric test should do. Culled the other two.

[ErikSchierboom]

@petertseng Does the updated canonical data now match your suggestions?

[petertseng]

I feel I have said what I wanted to say and that I do not object to the current situation. I remind others that I think it might still be possible to incorporate "by radius not by addition" (or whatever choice is chosen) if it will actually help a student understand what is wrong with a submitted implementation. if "just outside" (or whatever) is deemed to be sufficient, I accept the collective judgment.

[bencoman]

So btw, we are now at commit 11. Presuming some approval ticks aren't far away, when they arrive should I first squash commits in my branch before its merged into master?

[rpottsoh]

This should be squashed as you said or by whomever does the merge.

[SleeplessByte]

@bencoman this can just be squashed on merge (like @rpottsoh) says. No need to do anything :)

[ee7]

Yes, these commits should be squashed.

Reasons:

• Correctness. The commits are not standalone, and cannot be independently reverted. This can also interfere with git bisect.
• Brevity: it is easier for track maintainers to understand (and also eliminates an arguably needless merge commit).
• Usefulness: it automatically includes the PR number in the commit message, which makes git log more searchable.

See recent discussion at #1501 (comment).

Here is some recent repo history from git log --oneline.

Merge pull request #1534 from exercism/bencoman-anagram-grammar
anagram: grammar
Merge pull request #1526 from exercism/bencoman-patch-1
(origin/bencoman-patch-1) also fix surplus square bracket
fix missing comma
and bump version
Correctly add to existing comment
Robot simulator descriptions too long (#1525)
Improve anagram problem statement (#1529)
expand description, response to @sleeplessbyte suggestion
ETL descriptions too long

Note that it's even worse because there are robot-simulator and anagram changes in the middle of etl commits that come from a single PR.

The above is much harder to understand than the following (which represents the same changes):

anagram: Fix grammar (#1534)
robot-simulator: Shorten descriptions (#1525)
anagram: Improve problem statement (#1529)
etl: Shorten descriptions (#1526)

Of course, git log has useful flags like --no-merges and --graph . But maintainers may browse via GitHub, or not use such flags.

Please also use the standard commit message style.

[SleeplessByte]

@ee7 Thank you for this elaboration, and whilst appreciated, to squash or not to squash wasn't under debate. The question was if the OP needed to squash manually before merging. This PR isn't really the place to start a discussion which type of merge is best for this repository. In general, the people who merge often usually squash, per the reasons you're giving (so we're all with you here!).

standard commit message style

If you mean conventional commits: we don't use that in this repository, so it would be very out of place.

[ErikSchierboom]

@SleeplessByte Small bump asking for your review :)

[SleeplessByte]

I don't see the point in stripping the. It's only 4 characters (including the space) and further solidifies that there is only one inner, middle and outer circle.

[ErikSchierboom]

@bencoman FYI. There is a merge conflict in this PR.

Resolved

[rpottsoh]

Master was merged into the branch. Fixing.

[bencoman]

@rpottsoh What was wrong above? Clicking the button I used github's web editor to clean this up...

After your force-push the conflict still remains due to recent commit...
c5a1b8b75
How should this be handled?
(One thing I did forget was to re-bump the version after resolving the conflict)

[rpottsoh]

My fault. I was thinking that the PR should have been rebased when #1540 was merged. Resolving the conflict via the GitHub interface appears to bring along the changes in master that have occurred since branch bencoman-darts-slim-down-generated-method-names was created and now appear as changes that have been made in this PR. My intent was to eliminate all that so only the changes to darts/canonical-data.json appeared in the commit history. I wasn't successful at accomplishing what I had set out to do and ended up with master merged into bencoman-darts-slim-down-generated-method-names to resolve the conflict.

I did not fix version. I was just trying to undo the damage I was causing.

[bencoman]

I think I've cleared everything. Anything further @petertseng @rpottsoh @SleeplessByte ?

[SleeplessByte]

🚀 great work @bencoman . Thanks @ErikSchierboom and @rpottsoh for the reviews.