High Scores: small redesign to use an instance variable to hold the scores

[glennj]

Many of the Pharo exercises don't particularly utilize objects to hold state. This PR separates the methods to add scores from the queries.

Also a couple of additional tests to validate the returned scores are copies.

[glennj]

I don't know why the travis build still shows as Pending: the details end with "Done. Your build exited with 0"

[samWson]

Hi @glennj. I'm back from my travel so now I have the time to go over your pull requests. Sorry you had to wait so long.

I like your alternative of using an instance variable to hold state. It fits the Object Oriented paradigm better. Changing the tests to use the new API however presents a problem.

The tests are designed to the same specification that all the other language tracks use. For example this test case:

  "cases": [
    {
      "description": "List of scores",
      "property": "scores",
      "input": {
        "scores": [30, 50, 20, 70]
      },
      "expected": [30, 50, 20, 70]
    },

The above case defines a method/function called scores that takes a collection as an argument and returns another collection i.e. input and expected. All the language tracks work to this specification which keeps them predictably the same but as you have discovered might not be a good fit for the language paradigm.

We can add alternative solutions to our collection of exercises. In that case we could call this class HighScores2 (for an example see #392) . It would still need to pass the same tests i.e. same methods with the same arguments, which likely makes the use of an instance variable unnecessary.

The public API defined by the tests needs to stay the same as defined in the problem specification if we don't want to deviate from the rest of Exercism. I don't know if we have any flexibility to do this so I will raise another issue elsewhere to investigate this.

[glennj]

This seems like a pretty fundamental difference in interpretation of the canonical data.
I read the canonical data input as being more generally input to the test case, not as direct arguments for the property being tested.

The Ruby test suite does this:

  def test_list_of_scores
    # skip
    scores = [30, 50, 20, 70]
    expected = [30, 50, 20, 70]
    assert_equal expected, HighScores.new(scores).scores
  end

Similarly Python:

    def test_list_of_scores(self):
        scores = [30, 50, 20, 70]
        expected = [30, 50, 20, 70]
        self.assertEqual(HighScores(scores).scores, expected)

[samWson]

I see your point. If other language tracks are treating the problem specifications more generally then I am happy to as well.

There is one more step to do. Since you are changing the tests they will need to be regenerated according to these instructions. That will create the text files for the tests that are needed for the Exercism website. Once that is done then we can get this merged.

[bencoman]

I think Glen has a good point, but generation will overwrite HighScoresTest. To implement Glen's idea properly will need to change the test generation. I was doing some code-generation changes PR #372 before I got busy with some other work that interrupted the flow. Wow that was a while ago. I'll see if I can refresh that next week and close out that PR, then perhaps we can consider generating tests in more OO fashion. @macta, do you have any concerns with taking the design that way?

[glennj]

@samWson a side note: are you aware you have the Allergies exercise stuck in the mentor queue? https://exercism.io/mentor/solutions/92f15e6369ac49178c166b78c72adb50

[samWson]

@samWson a side note: are you aware you have the Allergies exercise stuck in the mentor queue? https://exercism.io/mentor/solutions/92f15e6369ac49178c166b78c72adb50

I see. I let this exercise slide ages ago. Simply put I couldn't solve it so I moved on to other exercises but I haven't done any exercises for months now. For now I'm wanting to spend more time on this repository as it has been left alone for too long. The previous mentor was inactive so that exercise was put back in the mentor queue.

[Bajger]

Looks ok, could be merged.

[Bajger]

@SleeplessByte: changes looks ok, but I don't see the conflicts to resolve merging PR. Can you see that?

[SleeplessByte]

@Bajger yes, the problem is that they are too large, that's why you can't see them here.

gh pr checkout 400

git remote add glennj https://github.com/glennj/exercism-pharo-smalltalk.git
git fetch glennj high-scores-more-OO
git switch --track glennj/high-scores-more-OO

And then merge in main

[glennj]

I recall the main reason these PRs have languished in the queue is that the process that generates the test classes from the problem specifications was not sufficiently flexible. I think the Pharo maintainers didn't want to keep exercises up-to-date by hand. Not having pursued joining the Pharo maintainers team, I've long since given up on these changes.

[Bajger]

@SleeplessByte, thanks for instructions, still I can't see merge conflicts. I have following:

gh pr status

Relevant pull requests in exercism/pharo-smalltalk

Current branch
  #400  High Scores: small redesign to use an instance ... [glennj:high-scores-more-OO]
  ✓ Checks passing - Review required

Created by you
  You have no open pull requests

Requesting a code review from you
  You have no pull requests to review

but if I do:

gh pr merge 400
X Pull request #400 is not mergeable: the merge commit cannot be cleanly created.
To have the pull request merged after all the requirements have been met, add the `--auto` flag.

Any thoughts? Maybe to rebase commits of PR to main branch? (It seems branch was created on master long time ago).

[SleeplessByte]

Yes, rebasing it will/should make the conflicts show up :)

[Bajger]

I don't have a write access to this branch, so I should fork from Glennj's repo and make own branch out of his branch? Or maybe adding me to .github/CODEOWNERS file would enable me to add more commits to this branch?

[Bajger]

@glennj : Is this exercise improvement something you'd like to discard? I can do new PR that would include these changes (I wasn't able to resolve conflicts with main).

[glennj]

Oops, that was an accident. Nevertheless, if you with to pursue it, you have my blessing.