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

Ruby/Resistor Colors: Add Mentor Notes #853

Merged
merged 2 commits into from
Mar 10, 2019
Merged

Conversation

emcoding
Copy link
Contributor

@emcoding emcoding commented Mar 10, 2019

We are about to merge the new side exercise Resistor Colors exercism/ruby#939. 🎉
I just want to get something ready for mentors. We can adjust after we've seen some submissions.

@exercism/website-copy-ruby

We are about to merge the new exercise Resistor Colors.
It's introduced as a side exercise first, so I don't expect much mentoring requests, but just in case, here's some preliminary Mentor Notes.  They will need evaluating after we've seen some solutions submitted. 

References:
Ruby PR https://github.com/exercism/ruby/pulls
Problem Specifications: exercism/problem-specifications#1466
This exercise is meant to be part of a series of Resistor exercises, building up to decoding up to 6 colors: exercism/problem-specifications#1458, and starting with just one color in Resistor Series nr 1
@emcoding emcoding marked this pull request as ready for review March 10, 2019 12:29
@SleeplessByte SleeplessByte requested a review from a team March 10, 2019 21:41
Copy link
Member

@pgaspar pgaspar 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 👍 Left a couple of notes for you to look over.

```

### General
This is the very first Array exercise, and the first loop. Introducing `map` is key in this exercise.
Copy link
Member

@pgaspar pgaspar Mar 10, 2019

Choose a reason for hiding this comment

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

This is the very first Array exercise

Isn't this set up as a bonus exercise? (reference)



### Changelog
First introduced 2019 Mar 10
Copy link
Member

Choose a reason for hiding this comment

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

Maybe we should have a specific format for the changelog. Maybe something like:

  • Version 1: exercise first introduced on March 10, 2019

Maybe HighScores can be a good guide as well?

Copy link
Member

@SleeplessByte SleeplessByte left a comment

Choose a reason for hiding this comment

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

Some comments inline


class ResistorColors

COLOR_CODES = ["black", "brown", "red", "orange", "yellow", "green", "blue", "violet", "grey", "white"]
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
COLOR_CODES = ["black", "brown", "red", "orange", "yellow", "green", "blue", "violet", "grey", "white"]
COLOR_CODES = %w[black brown red orange yellow green blue violet grey white].freeze

Default style via rubocop WordArray and rubocop MutableConstant.

Add below this solution that:

  • "Alternatives include writing out the array using brackets"; the talking points already mention this.
  • freeze should only be a talking point but not a reason to disapprove


### Minimal solution for approval
```ruby

Copy link
Member

Choose a reason for hiding this comment

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

I suggest removing the whitespace above class, but adding an empty line below the header, following default markdownlint.

Resistor Colors is a side exercise, unlocked by Hello World

### New Concepts
Array, Array#index, `map`, Constant, chaining methods
Copy link
Member

Choose a reason for hiding this comment

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

Add blank line between header and content, following default markdownlint

end

end

Copy link
Member

Choose a reason for hiding this comment

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

Remove whitespace between final end and end of code block.

end

```

Copy link
Member

Choose a reason for hiding this comment

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

Add note about acceptable variations include using a module, class << self, module_function and the likes. We don't want mentors to comment on any of that, seeing it's such an early exercise!

@emcoding
Copy link
Contributor Author

Thanks @SleeplessByte and @pgaspar. I'm merging it first, as I just want something ready for when the exercise is live. We can and will improve it later.

🍻

@emcoding emcoding merged commit 87c2c80 into master Mar 10, 2019
@pgaspar pgaspar deleted the ruby_resistor_colors_notes branch March 10, 2019 23:40
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.

3 participants