Skip to content

Improve descriptions to make sense per #1769#1777

Merged
ErikSchierboom merged 5 commits intomainfrom
fix-roman-numerals
Feb 4, 2021
Merged

Improve descriptions to make sense per #1769#1777
ErikSchierboom merged 5 commits intomainfrom
fix-roman-numerals

Conversation

@wolf99
Copy link
Copy Markdown
Contributor

@wolf99 wolf99 commented Jan 31, 2021

Fixes #1769

There were cases where the descriptions where the data did not numerically match the descriptions, such as

      "description": "1000 is a single M",
      "input": {
        "number": 1024
      },
      "expected": "MXXIV"
  1. Leave as-is
  2. Change the description to match the data
  3. Change the data to match the description

There is a slight semantic difference between 2 and 3 if you consider that what this case is testing is not just explicitly that 1024 is MXXIV, but that absent any other test to the effect it also implicitly tests that 1000 is M as the original description states.

I chose 2 as it accurately describes what the test is doing, but 3 is probably as valid a change

@petertseng petertseng removed their request for review February 1, 2021 14:37
@wolf99 wolf99 removed the correctness label Feb 1, 2021
@wolf99 wolf99 changed the title Improve descriptions per #1769 Improve descriptions to make sense per #1769 Feb 1, 2021
@wolf99 wolf99 requested review from a user and iHiD February 1, 2021 18:42
Copy link
Copy Markdown
Member

@ErikSchierboom ErikSchierboom left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@wolf99 I completely support these changes. They nicely follow our documentation that still says that the description is immutable. We've had some discussion on that in another issue (do you remember which issue @SleeplessByte?) and the conclusion there is that changing the description is fine too. Could you update the PR accordingly? I'll submit a different PR to update the documentation.

@wolf99
Copy link
Copy Markdown
Contributor Author

wolf99 commented Feb 2, 2021

We've had some discussion on that in another issue

Is in the review comments on #1747 I believe.

I will update my PR per your suggestion this evening then.

@wolf99 wolf99 force-pushed the fix-roman-numerals branch from 2a337b2 to d5c121b Compare February 2, 2021 21:29
{
"uuid": "d5b283d4-455d-4e68-aacf-add6c4b51915",
"description": "50 is a single L",
"description": "59 is a single LIX",
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need the is a single bit in the description? It looks like it was removed from other test cases.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Whoops, I missed this in the update.
Well caught Erik

Comment thread exercises/roman-numerals/canonical-data.json Outdated
Copy link
Copy Markdown
Member

@ErikSchierboom ErikSchierboom left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Copy Markdown

@valentin-p valentin-p left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

roman-numerals: [bug] canonical-data descriptions do not match data

4 participants