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

Add dnd_character exercise #1629

Merged
merged 7 commits into from
Feb 11, 2024
Merged

Add dnd_character exercise #1629

merged 7 commits into from
Feb 11, 2024

Conversation

ccadden
Copy link
Contributor

@ccadden ccadden commented Jan 27, 2024

Add dnd_character exercise based on 48in24 implementation status page: https://exercism.org/challenges/48in24/implementation_status

And on the forums: https://forum.exercism.org/t/add-dnd-character-exercise/9492

Copy link
Contributor

Hello. Thanks for opening a PR on Exercism 🙂

We ask that all changes to Exercism are discussed on our Community Forum before being opened on GitHub. To enforce this, we automatically close all PRs that are submitted. That doesn't mean your PR is rejected but that we want the initial discussion about it to happen on our forum where a wide range of key contributors across the Exercism ecosystem can weigh in.

You can use this link to copy this into a new topic on the forum. If we decide the PR is appropriate, we'll reopen it and continue with it, so please don't delete your local branch.

If you're interested in learning more about this auto-responder, please read this blog post.


Note: If this PR has been pre-approved, please link back to this PR on the forum thread and a maintainer or staff member will reopen it.

@github-actions github-actions bot closed this Jan 27, 2024
@iHiD iHiD reopened this Jan 27, 2024
@iHiD iHiD requested a review from kotp January 27, 2024 20:12
@kotp kotp assigned kotp and ccadden Jan 28, 2024
@kotp kotp added in progress x:module/practice-exercise Work on Practice Exercises x:status/claimed Someone is working on this issue labels Jan 28, 2024
@kotp kotp marked this pull request as draft January 29, 2024 05:18
@kotp
Copy link
Member

kotp commented Jan 29, 2024

I converted to draft as it is still in progress.

@ccadden ccadden force-pushed the add-dnd-exercise branch 2 times, most recently from c77f1eb to 6fe0cfe Compare January 31, 2024 02:24
@ccadden ccadden marked this pull request as ready for review January 31, 2024 02:26
@ccadden ccadden requested a review from kotp January 31, 2024 02:27
Comment on lines 52 to 116
(attributes << :hitpoints).each do |attribute|
assert_equal character.send(attribute), character.send(attribute)
end
Copy link
Member

Choose a reason for hiding this comment

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

I do not have much confidence that this is doing well what the test says it is doing.

    (attributes << :hitpoints).each do |attribute|
      expected = character.send(attribute)
      10.times do
        assert_equal expected, character.send(attribute)
      end

This would check each 10 (arbitrary choice) times, against what it was the answer for the first call


module Helpers
BASE_HITPOINTS = 10
CONSTITUTION_MODIFIER_TO_CONSTITUTION_MAP = {
Copy link
Member

Choose a reason for hiding this comment

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

This table seems to be ABILITY_SCORE_TO_MODIFIER.

Comment on lines 3 to 5
`dnd_character_test.rb` pass.
To get started with TDD, see the `README.md` file in your
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
`dnd_character_test.rb` pass.
To get started with TDD, see the `README.md` file in your
`dnd_character_test.rb` pass.
To get started with TDD, see the `README.md` file in your

@iHiD
Copy link
Member

iHiD commented Feb 3, 2024

@Mr-sigma @kotp Unless I'm missing something, the tests.toml and the tests themselves don't seem to be aligned. Compare this to C# for example: https://github.com/exercism/csharp/blob/main/exercises/practice/dnd-character/DndCharacterTests.cs

@kotp
Copy link
Member

kotp commented Feb 3, 2024

@Mr-sigma @kotp Unless I'm missing something, the tests.toml and the tests themselves don't seem to be aligned. Compare this to C# for example: https://github.com/exercism/csharp/blob/main/exercises/practice/dnd-character/DndCharacterTests.cs

The tests are not one to one in alignment, this is true. And CSharp having an exact one to one based on Problem Specification makes sense to me, since it is being generated, and it appears without any additional changes or removals from the Problem Specifications supplied information.

We should find that the tests do not need to be exact matches one to one, and may even be difficult to match, given the random nature of character generation, against a user written design, without likely driving a specific design or solution (and this is a practice exercise currently.)

The tests focus on the modification algorithm being applied, and the tests that attempt to ensure that the character attributes are changing randomly per character being generated, but also unchanging for the character once it is created.

We should find that the tests align in result, though, since deviation from that would be problematic if and when we decide to automate the tests, for example, and when bringing in problem descriptions from upstream.

Also, I think this pull request should still be in "Draft Mode" so we are still working through some things regarding the test and how it influences the solution.

@kotp kotp marked this pull request as draft February 3, 2024 18:29
@iHiD
Copy link
Member

iHiD commented Feb 3, 2024

@kotp
Copy link
Member

kotp commented Feb 4, 2024

My understanding is that tests should align 1-1 with the tests.toml.

I've looked through many other tracks in case I misunderstood this, and they are all 1-1 to problem specs:

I'll wait for @ErikSchierboom to weigh in on this on Tuesday, but my understanding of exercises Exercism-wide is that they should 1-1 align to tests.toml.

I will defer. When I was creating the generator, there were exercises that did not necessarily match up with Ruby, and I had made exceptions, but that was years ago, so it could have changed. I understand that many are like that, I just was not aware of the requirement to do so other than it being convenient to do so. (Some of the generators would conditional do things bringing in some tests, and making others or not do others.)

@ccadden ccadden requested a review from kotp February 10, 2024 01:24
@ccadden ccadden marked this pull request as ready for review February 10, 2024 01:24
Copy link
Member

@kotp kotp left a comment

Choose a reason for hiding this comment

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

Looking good. The one to one testing as mentioned, but the tests assertion arguments are in reverse order for many of the tests.

Comment on lines 10 to 12
def initialize; end

def self.modifier; end
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
def initialize; end
def self.modifier; end
def self.modifier
# Your Code Here
end
def initialize
# Your code here
end

include Helpers

def test_modifier_score_3
assert_equal DndCharacter.modifier(3), -4
Copy link
Member

Choose a reason for hiding this comment

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

In the tests, I believe it is supposed to be expected, actual in terms of the order of the statements.

We can verify this by setting them up to fail and seeing what the message is that is returned.

expected_hitpoints = BASE_HITPOINTS + DndCharacter.modifier(character.constitution)

attributes.each do |attribute|
assert_includes allowed_range, character.send(attribute)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
assert_includes allowed_range, character.send(attribute)
assert_includes allowed_range, character.send(attribute), "The expected value for #{attribute} is incorrect."

assert_includes allowed_range, character.send(attribute)
end

assert_equal expected_hitpoints, character.hitpoints
Copy link
Member

Choose a reason for hiding this comment

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

This one is in the correct order, the expected first, and then character hitpoints being actual.

Indeed, we could remove the expected_ from the name, or we could say only expected because the actual shows what kind of thing is expected based on the method being called on the object under test.

@ccadden ccadden force-pushed the add-dnd-exercise branch 2 times, most recently from 22fed10 to cf9a2dc Compare February 10, 2024 14:58
@ccadden
Copy link
Contributor Author

ccadden commented Feb 10, 2024

@kotp I added your patches, rebased, and ran rubocop -d -A to align with the repo's style guidelines. Should be all set to go, thanks for the feedback!

Copy link
Member

@kotp kotp left a comment

Choose a reason for hiding this comment

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

I sent a patch, but am approving early.

I will accept your choice on the what you see there and what you choose to apply.

This is definitely looking good.

@ccadden
Copy link
Contributor Author

ccadden commented Feb 11, 2024

@kotp, patch applied!

@ccadden ccadden requested a review from kotp February 11, 2024 13:03
Copy link
Member

@kotp kotp left a comment

Choose a reason for hiding this comment

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

Remove redundant inline disables.

exercises/practice/dnd-character/dnd_character_test.rb Outdated Show resolved Hide resolved
exercises/practice/dnd-character/dnd_character_test.rb Outdated Show resolved Hide resolved
@kotp kotp merged commit 0aa2712 into exercism:main Feb 11, 2024
4 checks passed
@kotp
Copy link
Member

kotp commented Feb 11, 2024

Thank you very much @Mr-sigma

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in progress x:module/practice-exercise Work on Practice Exercises x:status/claimed Someone is working on this issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants