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

darts: Port Darts Exercise #1019

Merged
merged 4 commits into from
Apr 25, 2020
Merged

darts: Port Darts Exercise #1019

merged 4 commits into from
Apr 25, 2020

Conversation

Thrillberg
Copy link
Contributor

This is my first exercise port! 👋 I think I followed the written instructions correctly, but one thing to note is that I, personally, came to doubt the educational value of this particular exercise as I worked through it. I'm submitting this PR in case others find it valuable but if someone wants to make an editorial decision to exclude this exercise, I would definitely not protest! 😄

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 think we are missing an entry to config.json.

Where do you see it fitting?

It may be a while before we bring this in, @F3PiX what do you think as well?

@Thrillberg
Copy link
Contributor Author

Thanks for taking a look, @kotp! Didn't realize it required an entry in config.json but I'd be happy to add one. Taking a glance at it, I can probably figure something out about how it works but how would I go about generating the uuid for the exercise? Or is there documentation about this that I'm missing (sorry if so!)

In terms of where it fits in, I'm not sure. In the JavaScript track it's rated as a difficulty "3" but has no topics listed, if we want to use that as guidance.

@kotp
Copy link
Member

kotp commented Jan 16, 2020

$ bin/configlet uuid --help
Generate a UUID.

Each Exercism exercise needs a unique and unmutable UUID. This needs to be
unique across the entire platform, not just within a track. In other words, if
you have 'clock' in Go and 'clock' in Haskell, they need to have different
UUIDs, even though they are based on the same problem specification.

Usage:
  bin/configlet uuid [flags]

Examples:
  bin/configlet uuid

Flags:
  -h, --help   help for uuid

That should help you out. More information is here, specifically the README.md.

@kotp
Copy link
Member

kotp commented Jan 16, 2020

In terms of where it fits in, I'm not sure. In the JavaScript track it's rated as a difficulty "3" but has no topics listed, if we want to use that as guidance.

Use your best guess, as you have created the proof of concept exercise...

@Thrillberg
Copy link
Contributor Author

Thanks for the guidance, @kotp! I've added an entry to config.json. I also noticed that CI is failing and I'm not sure if it's due to a change I made. If so, do you happen to have any ideas on what is the cause?

@kotp
Copy link
Member

kotp commented Jan 20, 2020

Thanks for the guidance, @kotp! I've added an entry to config.json. I also noticed that CI is failing and I'm not sure if it's due to a change I made. If so, do you happen to have any ideas on what is the cause?

Traceback (most recent call last):
	1: from /tmp/darts20200119-6390-w47l5w/darts_test.rb:2:in `<main>'
/tmp/darts20200119-6390-w47l5w/darts_test.rb:2:in `require_relative': /tmp/darts20200119-6390-w47l5w/darts.rb:22: syntax error, unexpected => (SyntaxError)
      10.0.. => 0,
             ^~
/tmp/darts20200119-6390-w47l5w/darts.rb:29: syntax error, unexpected end-of-input, expecting keyword_end
rake aborted!
Command failed with status (1): [/home/travis/.rvm/rubies/ruby-2.5.5/bin/ru...]
      10.0.. => 0,

This line (22) is causing a problem with the version of Ruby being ran. Ensure that the tests are Ruby 2.5 friendly.

exercises/darts/.meta/solutions/darts.rb Outdated Show resolved Hide resolved
@Thrillberg
Copy link
Contributor Author

That seems to have done it! Thanks, @kotp!

@kotp kotp self-requested a review January 22, 2020 19:57
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.

Looks good, some suggestions for the "copy".

Let me know if you think the changes are worthwhile... otherwise I would approve it as is. If the changes are not necessary, let me know and I will approve as is.

exercises/darts/README.md Show resolved Hide resolved

The outer circle has a radius of 10 units (This is equivalent to the total radius for the entire target), the middle circle a radius of 5 units, and the inner circle a radius of 1. Of course, they are all centered to the same point (That is, the circles are [concentric](http://mathworld.wolfram.com/ConcentricCircles.html)) defined by the coordinates (0, 0).

Write a function that given a point in the target (defined by its `real` cartesian coordinates `x` and `y`), returns the correct amount earned by a dart landing in that point.
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
Write a function that given a point in the target (defined by its `real` cartesian coordinates `x` and `y`), returns the correct amount earned by a dart landing in that point.
Write a method that, given a point in the target (defined by its `real` Cartesian coordinates `x` and `y`), returns the correct amount earned by a dart landing in that point.

Cartesian is a name, and capitalized. The use of a comma is consistent and works because without the explanation would make a complete sentence itself.

Copy link
Member

Choose a reason for hiding this comment

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

The change from "function" to "method" is a local Ruby change, and not suitable for the Readme in problem-specifications, while the missing comma should be suggested at the original source.

* If the dart lands in the middle circle of the target, player earns 5 points.
* If the dart lands in the inner circle of the target, player earns 10 points.

The outer circle has a radius of 10 units (This is equivalent to the total radius for the entire target), the middle circle a radius of 5 units, and the inner circle a radius of 1. Of course, they are all centered to the same point (That is, the circles are [concentric](http://mathworld.wolfram.com/ConcentricCircles.html)) defined by the coordinates (0, 0).
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
The outer circle has a radius of 10 units (This is equivalent to the total radius for the entire target), the middle circle a radius of 5 units, and the inner circle a radius of 1. Of course, they are all centered to the same point (That is, the circles are [concentric](http://mathworld.wolfram.com/ConcentricCircles.html)) defined by the coordinates (0, 0).
The outer circle has a radius of 10 units (This is equivalent to the total radius for the entire target), the middle circle a radius of 5 units, and the inner circle a radius of 1. They are all centered to the same point (That is, the circles are [concentric](http://mathworld.wolfram.com/ConcentricCircles.html)) defined by the coordinates (0, 0).

The "Of course, " can be removed as we are providing the course.

Copy link
Member

Choose a reason for hiding this comment

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

This can be suggested at the source, rather than locally here.

@Thrillberg
Copy link
Contributor Author

I entirely agree with all your comments. The reason I didn't do such proofreading on my own is that I generated the README from the problem specifications. Should I make changes to the README here, just for the Ruby implementation of the exercise, or should I propose the changes on the problem specifications description: https://github.com/exercism/problem-specifications/blob/master/exercises/darts/description.md ?

kotp
kotp previously approved these changes Jan 24, 2020
@kotp
Copy link
Member

kotp commented Jan 24, 2020

You are right. The suggestions should be at the source. The changes here are going to be the Ruby specific change of function changed to method. I approved this PR, but if you want to change that word to be specifically correct for the Ruby track, that would be great.

I know when the source is changed and we re-generate, that this will be "incorrect" again.

@Thrillberg
Copy link
Contributor Author

Thrillberg commented Jan 25, 2020

The changes here are going to be the Ruby specific change of function changed to method

Hmm yeah this is an interesting nuance. I'm inclined not to make the change here since, as you pointed out, it will get reverted back to function whenever we re-generate. Totally coincidentally I was walking a beginner through setting up Exercism on their computer and encountered the word function in the hello_world exercise in Ruby, too (https://github.com/exercism/ruby/blob/master/exercises/hello-world/README.md).

So maybe it's a losing battle to try to keep the terminology different between Ruby and other languages and they should just all be called functions?

Anyway, I'd be happy to propose the other (imo more important) suggestions you make at the source, which should be universally helpful regardless of programming language. Does that sound alright?

EDIT: I just read a little further in the problem-specifications repo and found this: exercism/problem-specifications#1560 I imagine it is locked to a change such as this. Given that, should we make the wording improvement changes locally in this repo anyway?

@kotp
Copy link
Member

kotp commented Jan 25, 2020

Yeah, I am for making the changes local. As we work on the content for the Ruby track, and update, we will notice when things slip (or not) and have regressions, but hopefully we will note them, perhaps at some point make some changes that help us with track specific language so it is less manual, less likely to revert.

@Thrillberg
Copy link
Contributor Author

Great! I've just made the wording changes. What do you think?

exercises/darts/README.md Show resolved Hide resolved
@kotp kotp merged commit 7033db2 into exercism:master Apr 25, 2020
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