Skip to content

Refactor exercises to match canonical data #1762

Closed

Description

@cmccandless

There are a number of exercises that fall into one of the following cases:

Function names that differ from those specified in the canonical data

armstrong-numbers uses is_armstrong instead of is_armstrong_number:

def is_armstrong(number):
return sum(pow(int(d), len(str(number))) for d in str(number)) == number

From the canonical-data:

    {
      "description": "Zero is an Armstrong number",
      "property": "isArmstrongNumber",
      "input": {
        "number": 0
      },
      "expected": true
    },

*Note: in these cases, the camelCase names used in the canonical data should be converted to snake_case names in the tests, example solution, and solution stub.

Test input format differs from canonical data

tournament passes a multiline string instead of a list of single-line strings.

def test_there_can_be_more_than_one_winner(self):
results = ('Allegoric Alaskans;Blithering Badgers;loss\n'
'Allegoric Alaskans;Blithering Badgers;win')
table = ('Team | MP | W | D | L | P\n'
'Allegoric Alaskans | 2 | 1 | 0 | 1 | 3\n'
'Blithering Badgers | 2 | 1 | 0 | 1 | 3')
self.assertEqual(tally(results), table)

From the canonical-data:

    {
      "description": "There can be more than one winner",
      "property": "tally",
      "input": {
        "rows": [
          "Allegoric Alaskans;Blithering Badgers;loss",
          "Allegoric Alaskans;Blithering Badgers;win"
        ]
      },
      "expected": [
        "Team                           | MP |  W |  D |  L |  P",
        "Allegoric Alaskans             |  2 |  1 |  0 |  1 |  3",
        "Blithering Badgers             |  2 |  1 |  0 |  1 |  3"
		]
	},

Test output format differs from canonical data

saddle-points expects a set of tuples instead of a list of dict objects

def test_identify_single_saddle_point(self):
matrix = [[9, 8, 7], [5, 3, 2], [6, 6, 7]]
self.assertEqual(saddle_points(matrix), set([(2, 1)]))

From the canonical-data:

    {
      "description": "Can identify single saddle point",
      "comments": [
        "This is the README example."
      ],
      "property": "saddlePoints",
      "input": {
        "matrix": [
          [9, 8, 7],
          [5, 3, 2],
          [6, 6, 7]
        ]
      },
      "expected": [
        {
          "row": 2,
          "column": 1
        }
      ]
    },

Test input order differs from canonical data

change uses the order target, coins instead of coins, target

def test_single_coin_change(self):
self.assertEqual(find_minimum_coins(25, [1, 5, 10, 25, 100]), [25])

From the canonical-data:

    {
      "description": "single coin change",
      "property": "findFewestCoins",
      "input": {
        "coins": [1, 5, 10, 25, 100],
        "target": 25
      },
      "expected": [25]
    },

In an effort to simplify the job of the WIP test generator, it would be beneficial to reduce these inconsistencies now, so that it does not become the job of the generator PR to correct example solutions and stubs that do not match newly generated tests.

Here is the full list of exercises in which this occurs

  • alphametics
  • armstrong-numbers
  • binary-search
  • bob
  • book-store
  • change
  • collatz-conjecture
  • crypto-square
  • diamond
  • difference-of-squares
  • dominoes
  • gigasecond
  • grains
  • grep
  • isbn-verifier
  • knapsack
  • leap
  • list-ops
  • markdown
  • meetup
  • minesweeper
  • nth-prime
  • palindrome-products
  • prime-factors
  • pythagorean-triplet
  • raindrops
  • rectangles
  • roman-numerals
  • saddle-points
  • secret-handshake
  • sieve
  • spiral-matrix
  • sublist
  • sum-of-multiples (foregoing updates for now)
  • tournament
  • transpose
  • triangle
  • two-fer
  • word-count
  • wordy
  • yacht

This issue may be considered resolved once all of the above issues have been updated to match the canonical properties.

Activity

  1. spencer51324312 commented on May 9, 2019

    @spencer51324312
    Contributor

    @cmccandless Do we keep this checklist up to date?

  2. cmccandless commented on May 9, 2019

    @cmccandless
    ContributorAuthor

    @spencer51324312 I attempt to, yes.

  3. BethanyG commented on May 19, 2019

    @BethanyG
    Member

    @cmccandless - I would love to help with this, but want to make sure I do so in the "right" way. Should I just dig into whichever non-checked off exercise is next on the list and have at it - one pull per exercise changed?

  4. cmccandless commented on May 20, 2019

    @cmccandless
    ContributorAuthor

    @BethanyG

    Should I just dig into whichever non-checked off exercise is next on the list and have at it - one pull per exercise changed?

    Perfect!

  5. BethanyG commented on May 22, 2019

    @BethanyG
    Member

    So I am going to call out binary-search. I really don't think that the cannonical.json (which wants a function named find) is a good idea here. A find function could shadow Python's str.find().
    So - does the cannonical data change, does the exercise, or is this an exception? https://github.com/exercism/problem-specifications/blob/master/exercises/binary-search/canonical-data.json

  6. BethanyG commented on May 22, 2019

    @BethanyG
    Member

    ..on second thought, I will go ahead and do the changes, and if the answer is no, we can just kill the pull request.

  7. Grociu commented on May 30, 2019

    @Grociu
    Contributor

    I did a check for the exercise "two-fer", everything seems to be in order with that one, reflects the canonical data as is in my opinion.

  8. cmccandless commented on May 31, 2019

    @cmccandless
    ContributorAuthor

    @Grociu regarding two-fer, the mismatch is in the choice to use name="" as the null case rather than name=None

  9. Grociu commented on May 31, 2019

    @Grociu
    Contributor

    canonical-data.json for the sum_of_multiples exercise uses sum as the main function name. As it is a built-in function name, and that function is actually used in the example.py do I go with sum and rewrite the example, or something else?

    What I did so far is from sum_of_multiples import sum_of_multiples as sum into the test function and make the test cases test the imported sum() function. I don't know if that's up to the standard.

  10. cmccandless commented on May 31, 2019

    @cmccandless
    ContributorAuthor

    @Grociu You might consider storing the builtin definition of sum() in another variable, i.e.

    old_sum = sum
    def new_sum(nums, maximum):
        return old_sum(...)
    
    sum = new_sum
  11. BethanyG commented on Jun 1, 2019

    @BethanyG
    Member

    For palindrome-products I have a few issues / questions:

    1. The current Travis tests fail if I remove from __future__ import division from example.py ... but since we are EOL Python 2.7, we really should be removing it. Should we undertake that now, or shelve for when the EOL Python 2.7 gets rolling in earnest?

    2. Found the following as a helper method in the tests:

    def assertFactorsEqual(self, actual, expected):
        self.assertEqual(set(map(frozenset, actual)),
                         set(map(frozenset, expected)))

    which effectively negates any data type returned by the code under test (provided that the data type is an iterable that can be made into a set of frozensets. ) While this gives wide latitude for exercise submissions, it does feel as though it's circumventing the point of the exercise (and the canonical-data), which is to return a list of factor lists. I did change the data types for the expected results, but did not remove the helper method. Should I have?

    1. My Flake8 complained that the example.py solution was "too complex", but I left it as-is. The palindromes function has two additional nested functions within it, which is a bit much. Should we be considering a re-factor?
  12. Grociu commented on Jun 20, 2019

    @Grociu
    Contributor

    For wordy: Do you want me to rewrite the tests so they are checking the "raises error with message"
    cases whether they raise the actual error messages ("unknown operation", "syntax error") as stated in canonical-data.json?
    I've been toying with this approach a little, and it would require rewriting the example somewhat.

  13. cmccandless commented on Jun 20, 2019

    @cmccandless
    ContributorAuthor

    Nope; the decision was made to not require students to use exact error message, only to have a message present. It's up to mentors then to comment on whether the exception messages match the case.

  14. Grociu commented on Jun 20, 2019

    @Grociu
    Contributor

    OK, thanks. Can you point me towards the reason this is not marked as done? I see an inconsistency in declaring strings 'text' vs "text" in the test function, and twice the indentation can be a little better. I have these fixed in my local branch, but I feel like I'm missing something.

  15. cmccandless commented on Jun 20, 2019

    @cmccandless
    ContributorAuthor

    I picked up on most of those via a script; it looks like that one might've been a false positive. I've checked it off.

  16. cmccandless commented on Jul 1, 2019

    @cmccandless
    ContributorAuthor

    All items on the above list have been addressed, so I'm closing this issue.

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

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions