armstrong-numbers: port by guygastineau · Pull Request #988 · exercism/ruby

guygastineau · 2019-08-08T01:01:20Z

I have rebased the last ten commits of @Tuxified's work from #893 onto the current master.
canonical-data.json

guygastineau · 2019-08-08T01:17:00Z

I intend to make a test generator for this. It has been awhile since I have done so. I should have some work up for that later tonight or tomorrow.

guygastineau · 2019-08-08T02:38:45Z

At first I thought I would add integer delimiting into the generator. Then I was sad when I realized that converting them back into integers destroys the formatting.

Then I felt silly when I realized that the generator needs it in a string 🤣

exercises/armstrong-numbers/.meta/generator/armstrong_numbers_case.rb

guygastineau · 2019-08-08T03:16:36Z

Okay, I think I am finished messing with this. 😄

emcoding

Hi @guygastineau Thanks for bringing the exercise back to live :-)

emcoding · 2019-08-08T09:14:17Z

config.json

+      "slug": "armstrong-numbers",
+      "uuid": "77c5c7e6-265a-4f1a-9bc4-851f8f825420",
+      "core": false,
+      "unlocked_by": "isogram",


@guygastineau I didn't realize before that Isogram has moved since the last time we discussed adding this exercise. Let's make it

Suggested change

"unlocked_by": "isogram",

"unlocked_by": "series",

Sorry that I missed this.

Note to self: without the lambda, it could be a level 1 side exercise. Then, it may need a more step by step help in the readme.

(This doesn't need to be addressed in this PR, unless you'd prefer to do so, Guy. )

Great, I figured there might need to be some changes to it in the config.json.
I'm on it 😃

I mean, I will change it to unlock from series I don't especially feel like writing step-by-step instructions for closures in a lvl 1 exercise readme extension.

emcoding · 2019-08-08T09:31:38Z

exercises/armstrong-numbers/.meta/solutions/armstrong_numbers.rb

+  def self.include?(number)
+    raise_to_length = ->(n) { n ** number.digits.size }
+    number.digits.sum(&raise_to_length) == number
+  end


:-)
Since the solution file is sometimes consulted by (new) mentors and students, and to avoid miscommunications in general, I'd prefer to have a solution that is in sync with the solution that we strive for.
That's mostly a matter of opinion, I don't think we have a policy for this.

And I'm slightly confused too. ;-) Does this mean that you think the original solution is not good enough? Or?

Oh, I just really wanted to use stabby lambda for the hell of it, and the guide for porting exercises on this track says the solution doesn't need to be ideal (although I think this one is fun 😃)

I have no problem reverting it. That is a simple fix 😃

(I think that porting guide was written before the profound track anatomy project happened, so wanting example solutions that fit the level of the exercise is a reasonable desire given all that you all have done to polish this track 😃)

The example solution is a proof of concept, and if it is not "normal" this means that we will likely spot it quickly, if for some reason it comes to be a solution submitted by a student. So there are some advantages to having a non ideal solution, as long as it "proves the concept".

On the other hand, since this isn't example.rb it may be being presented in the note as an acceptable solution... one to approve. In that situation, the story changes.

Oh, I just really wanted to use stabby lambda for the hell of it

Yeah, you mentioned the lambda fun somewhere else, so that's what I thought it was 🤓 .

FYI:
My thinking is: The beauty of this exercise to me is that it does 'light math', that can be solved with 'early' Ruby methods without being scary mathy. And then it can be refactored to use digits.sum.
For other purposes, this exercise would need more 'progressions' in other aspects, to make the learning curve between exercises steep enough on the higher levels.

Thanks for reverting :-)

guygastineau · 2019-08-08T13:47:01Z

Also, I noticed that you all have a very explicit merge history here. I am very impressed 😍

This PR has a load of commits, so I would be more than happy to rebase it into just a couple commits.
I would probably squash @Tuxified's commits into 1 (might use fixup on one or two that aren't relevant to the final product here), and I can squash mine together, but I'll fixup or drop any that don't need to be there (like the one introducing the lambda).

Anyway, this is just an idea to keep both contributors in the history without having a 20 commit merge for this simple exercise. Let me know what you all would prefer, and if I should do anything about it 😄

kotp · 2019-08-08T16:40:08Z

exercises/armstrong-numbers/.meta/solutions/armstrong_numbers.rb

+  def self.include?(number)
+    exponent = number.digits.size
+
+    number.digits.sum { |digit| digit ** exponent } == number


Don't use spaces around ** as it emulates how we expect to see it (similar to x^y, very close together), and makes it clear it isn't a typographical error due to style showing intent.

I'll change it, but I strongly disagree. I think operators should always have spaces in languages where it is possible, but I would not consider myself a real rubyist yet (, so I defer to your opinion).

In case it's a comfort: no spaces around exponents is recommended by one of the Ruby Styleguides. Not saying I like it, but I settle for consistency :-)
https://rubystyle.guide/#spaces-operators

kotp · 2019-08-08T16:42:03Z

exercises/armstrong-numbers/.meta/solutions/armstrong_numbers.rb

+  def self.include?(number)
+    raise_to_length = ->(n) { n ** number.digits.size }
+    number.digits.sum(&raise_to_length) == number
+  end


The example solution is a proof of concept, and if it is not "normal" this means that we will likely spot it quickly, if for some reason it comes to be a solution submitted by a student. So there are some advantages to having a non ideal solution, as long as it "proves the concept".

kotp · 2019-08-08T16:43:02Z

exercises/armstrong-numbers/.meta/solutions/armstrong_numbers.rb

+  def self.include?(number)
+    raise_to_length = ->(n) { n ** number.digits.size }
+    number.digits.sum(&raise_to_length) == number
+  end


On the other hand, since this isn't example.rb it may be being presented in the note as an acceptable solution... one to approve. In that situation, the story changes.

exercises/armstrong-numbers/.meta/generator/armstrong_numbers_case.rb

guygastineau · 2019-08-08T16:56:59Z

So, which solution do you all want?

pgaspar

Good work! Added a couple comments on ways I think we can simplify the generator 👍

This PR has a load of commits, so I would be more than happy to rebase it into just a couple commits.

Thank you - this is appreciated! Squashing this to two commits as you are suggesting sounds great to me.

exercises/armstrong-numbers/.meta/generator/armstrong_numbers_case.rb

guygastineau · 2019-08-08T19:15:51Z

Thanks for all the pointers everybody 😄

I will wait to squash into two commits until the work is approved (so I don't have to do it twice).

guygastineau · 2019-08-08T20:44:29Z

Thanks @kotp

I'll rebase this after dinner.

pgaspar

Thank you!

- not using Integer.digits as that is only available on Ruby 2.4

- fix conflicts - implement generator

guygastineau · 2019-08-08T22:50:00Z

I suppose I could have done some squashes mixed in with the fixups, but at least both contributors are represented in the history (which was my main goal here).

guygastineau · 2019-08-08T22:51:29Z

It's ready for you all now. Thanks for working with me on this ;)

Are there any exercises that need generators? I think there used to be a file or an issue somewhere showing which ones needed a generator implemented. I'm happy to hunt them down and make them ;)

guygastineau mentioned this pull request Aug 8, 2019

Add Armstrong Numbers exercise #893

Closed

1 task

guygastineau changed the title ~~WIP armstrong-numbers: port~~ armstrong-numbers: port Aug 8, 2019

guygastineau commented Aug 8, 2019

View reviewed changes

exercises/armstrong-numbers/.meta/generator/armstrong_numbers_case.rb Outdated Show resolved Hide resolved

emcoding reviewed Aug 8, 2019

View reviewed changes

emcoding requested review from kotp and pgaspar August 8, 2019 09:46

kotp reviewed Aug 8, 2019

View reviewed changes

pgaspar suggested changes Aug 8, 2019

View reviewed changes

exercises/armstrong-numbers/.meta/generator/armstrong_numbers_case.rb Outdated Show resolved Hide resolved

exercises/armstrong-numbers/.meta/generator/armstrong_numbers_case.rb Outdated Show resolved Hide resolved

guygastineau commented Aug 8, 2019

View reviewed changes

exercises/armstrong-numbers/.meta/generator/armstrong_numbers_case.rb Show resolved Hide resolved

kotp previously approved these changes Aug 8, 2019

View reviewed changes

pgaspar previously approved these changes Aug 8, 2019

View reviewed changes

Tuxified and others added 2 commits August 8, 2019 18:42

Add Armstrong Numbers exercise

0620959

- not using Integer.digits as that is only available on Ruby 2.4

armstrong-numbers: finish implementation

fe88c70

- fix conflicts - implement generator

guygastineau dismissed stale reviews from pgaspar and kotp via fe88c70 August 8, 2019 22:46

guygastineau force-pushed the armstrong-numbers branch from 23f9242 to fe88c70 Compare August 8, 2019 22:46

kotp approved these changes Aug 8, 2019

View reviewed changes

kotp merged commit 27126d8 into exercism:master Aug 8, 2019

guygastineau deleted the armstrong-numbers branch August 9, 2019 01:46

Uh oh!

Conversation

guygastineau commented Aug 8, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

guygastineau commented Aug 8, 2019

Uh oh!

guygastineau commented Aug 8, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

guygastineau commented Aug 8, 2019

Uh oh!

emcoding left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

emcoding Aug 8, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

emcoding Aug 8, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

guygastineau commented Aug 8, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

guygastineau commented Aug 8, 2019

Uh oh!

pgaspar left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

guygastineau commented Aug 8, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

guygastineau commented Aug 8, 2019

Uh oh!

pgaspar left a comment

Choose a reason for hiding this comment

Uh oh!

guygastineau commented Aug 8, 2019

Uh oh!

guygastineau commented Aug 8, 2019

Uh oh!

Reviewers

Assignees

guygastineau commented Aug 8, 2019 •

edited

Loading

guygastineau commented Aug 8, 2019 •

edited

Loading

emcoding Aug 8, 2019 •

edited

Loading

emcoding Aug 8, 2019 •

edited

Loading

guygastineau commented Aug 8, 2019 •

edited

Loading

guygastineau commented Aug 8, 2019 •

edited

Loading