Skip to content
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

HighScore: Two new tests for methods interference #1744

Closed
wants to merge 2 commits into from

Conversation

simmol
Copy link
Contributor

@simmol simmol commented Apr 9, 2019

When solving the problem event using the tests.
Is very easy to sort the scores in personal_top_tree() or personal_best() which will break latest().
The test never catch that because they are testing only one method at a time.

But knowing the difference between sorting in place and returning new sorted list is valuable.
Also is good to know early you have to be careful with objects states.

When solving the problem event using the tests.
Is very easy to sort the scores in personal_top_tree() or personal_best() which will break latest().
The test never catch that because they are testing only one method at a time.

But knowing the difference between sorting in place and returning new sorted list is valuable.
Also is good to know early you have to be careful with objects states.
@simmol simmol requested a review from a team as a code owner April 9, 2019 06:28
@cmccandless
Copy link
Contributor

This issue has come up before in #1735.

Currently, side effect testing is outside the scope of this exercise. There is a discussion here to include such tests, but I would like to wait and see the consensus there before adding these tests here.

@simmol
Copy link
Contributor Author

simmol commented Apr 9, 2019

@cmccandless
What is the scope of the exercise ?

Because for me at least if we introduce classes, data integrity is the lesson you need to learn as early as possible.
If the main point is list manipulation, that list is mutable is very valid point.

So for me this kind of tests are logical to exist.
I will see the Issue you mentioned and write there also.

@cmccandless
Copy link
Contributor

Looking at the original PR that created the exercise (exercism/problem-specifications#1378), the intent was for an easy exercise for working with arrays/lists. Some tracks don't bother with a class implementation at all for this exercise. I'm wondering if that would be a better route for this track as well.

@simmol
Copy link
Contributor Author

simmol commented Apr 9, 2019

That is something that bugged me also when i saw it for the first time :)
Normally python is learned trough extensive use of functions and then moving to classes.

Changing the exercise to produce just three functions that manipulate list in different manners make sense. I am not sure how to handle that change tough ?
Isn't the Description of the exercise the same for different tracks ?

Sorry to bother you with so many questions.
I am still not sure how the whole system is organised.

If you like we can talk it trough in the maintenance channel i am willing to put some effort to improve/fix this exercise and tests. It is one of the first people see and do.

@cmccandless
Copy link
Contributor

A PR that converts to using functions would be welcome. I would close this PR first though. Feel free to reach out on Slack if you need help getting started.

@simmol simmol closed this Apr 10, 2019
@simmol
Copy link
Contributor Author

simmol commented Apr 10, 2019

I will do some work to change the exercise to use functions instead of class with 3 functions.
This should simply it for learners and mentor a like.

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.

2 participants