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

Sync tests for practice exercise space age #2640

Merged
merged 3 commits into from
Jan 18, 2024

Conversation

manumafe98
Copy link
Contributor

@manumafe98 manumafe98 commented Jan 13, 2024

pull request

This issue addresses: #2388

This one required a major reformat, due to tests passing a new parameter and also usage of a deprecated method setScale

{
  "uuid": "57b96e2a-1178-40b7-b34d-f3c9c34e4bf4",
  "description": "invalid planet causes error",
  "property": "age",
  "input": {
    "planet": "Sun",
    "seconds": 680804807
  },
  "expected": {
    "error": "not a planet"
  }
}

I used as reference the go track that is updated.

Reviewer Resources:

Track Policies

Copy link
Contributor

@sanderploegsma sanderploegsma left a comment

Choose a reason for hiding this comment

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

This is a good learning opportunity to show that sometimes we can choose not to implement a test from the canonical data: the design of the exercise is such that it doesn't allow any unknown planets, so the extra test case isn't applicable at all.

I double-checked how the C# implementation handled this and they chose not to include this test case. In my opinion we should follow their lead here, since the exercises are very similar.

So to conclude, please revert all changes and instead mark the new test as "not included" in .meta/tests.toml, along with a comment explaining why. It should look something like this:

[57b96e2a-1178-40b7-b34d-f3c9c34e4bf4]
description = "invalid planet causes error"
include = false
comment = "Excluded because the design of the exercise does not allow for arbitrary planets"

@manumafe98
Copy link
Contributor Author

Perfect! Would be great if Exercism does the same across all tracks i guess? Because for example in the Go implementation it is applied.

@sanderploegsma
Copy link
Contributor

Perfect! Would be great if Exercism does the same across all tracks i guess? Because for example in the Go implementation it is applied.

Well that is because they chose to implement it with a different method signature. If our exercise would have looked similar to that one, we might have chosen to include the test that checks for an invalid planet.

It is up to each track to choose how to implement each practice exercise according to that programming language's best practices. For Go, the choice of accepting a planet argument makes sense as it's not as object-oriented as Java and C#. It also doesn't really have the concept of enums, which would have been another way to restrict the values it accepts at compile-time.

@manumafe98
Copy link
Contributor Author

Perfect! Would be great if Exercism does the same across all tracks i guess? Because for example in the Go implementation it is applied.

Well that is because they chose to implement it with a different method signature. If our exercise would have looked similar to that one, we might have chosen to include the test that checks for an invalid planet.

It is up to each track to choose how to implement each practice exercise according to that programming language's best practices. For Go, the choice of accepting a planet argument makes sense as it's not as object-oriented as Java and C#. It also doesn't really have the concept of enums, which would have been another way to restrict the values it accepts at compile-time.

Great! just in case i've maintained the removal of the depracated method setScale and applied what you suggested me to the tests in the .meta/tests.toml

Copy link
Contributor

@sanderploegsma sanderploegsma left a comment

Choose a reason for hiding this comment

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

This looks great!

@sanderploegsma sanderploegsma merged commit c4d42d7 into exercism:main Jan 18, 2024
5 checks passed
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