Skip to content

Implement Resistor Colors exercise#808

Merged
sshine merged 1 commit intoexercism:masterfrom
tejasbubane:implement-resistor-colors-exercise
Mar 12, 2019
Merged

Implement Resistor Colors exercise#808
sshine merged 1 commit intoexercism:masterfrom
tejasbubane:implement-resistor-colors-exercise

Conversation

@tejasbubane
Copy link
Copy Markdown
Member

Problem Specification: exercism/problem-specifications#1466

@tejasbubane tejasbubane force-pushed the implement-resistor-colors-exercise branch from 8d065e5 to b5410fb Compare March 9, 2019 18:35
@tejasbubane
Copy link
Copy Markdown
Member Author

tejasbubane commented Mar 10, 2019

I keep getting this segmentation fault while running the ensure-readmes-updated script:

➜ bin/ensure-readmes-are-updated.sh
Cloning into 'problem-specifications'...
remote: Enumerating objects: 6, done.
remote: Counting objects: 100% (6/6), done.
remote: Compressing objects: 100% (6/6), done.
remote: Total 8462 (delta 0), reused 6 (delta 0), pack-reused 8456
Receiving objects: 100% (8462/8462), 1.70 MiB | 493.00 KiB/s, done.
Resolving deltas: 100% (4986/4986), done.
Checking readme for resistor-colors
bin/ensure-readmes-are-updated.sh: line 20:  1144 Segmentation fault: 11  $configlet generate . --only "$exercise" --spec-path "problem-specifications"

PS: It runs fine on master, but fails on this branch.

@petertseng
Copy link
Copy Markdown
Member

Since I couldn't find output containing segmentation fault in the Travis CI output, I am taking your word for it.
Since the output you have displayed says that configlet is segfaulting, it sounds like the thing to do is to find a minimal set of conditions to reproduce this and then report it to https://github.com/exercism/configlet/issues

Since I don't use configlet I regret being unable to assist in the specifics of that

Copy link
Copy Markdown
Contributor

@sshine sshine 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 need some kind of error handling built in.

I like this exercise over allergies for the reason that you don't need exponentiation, so it's less mathy.

If convert Black == 0, then what is value []? (This is perhaps a question better asked on problem-specifications.)

convert Grey = 8
convert White = 9

value :: [String] -> Int
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Since we're converting ["black", "brown"], I think it would make sense to

  1. Have error handling.
  2. Return an Integer.

Since it makes sense to have error handling, it also makes sense to ask if we should go with Maybe or Either.

Then readMaybe or readEither from Text.Read can come into use.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

From the tests in specification, I assumed the input will always be valid. Let me know if I should make this change or wait for more discussion (in the problem spec as well).

@sshine
Copy link
Copy Markdown
Contributor

sshine commented Mar 11, 2019

bin/ensure-readmes-are-updated.sh: line 20:  1144 Segmentation fault: 11  $configlet generate . --only "$exercise" --spec-path "problem-specifications"

That's strange, line 20 only contains git, grep, cut and sort.

configlet isn't run until line 28.

What if you echo the line that begins with $configlet and run that manually?

It sounds like configlet fails because of the way it's compiled...

deriving (Eq, Show, Read)

convert :: Color -> Int
convert Black = 0
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I notice that it would be possible to use http://hackage.haskell.org/package/base-4.12.0.0/docs/Prelude.html#t:Enum here.

I have not yet determined whether it would be a good idea to do so.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I had the same thought.

What you don't get with deriving Enum is an explicit annotation that Black is 0, etc.

Making an explicit instance Enum where ... has the drawback that you need to specify both toEnum and fromEnum, even though the exercise only needs the one. There are ways to avoid that, but the first way (using a dynamic lookup table) should be discouraged, and the second way (using Data and Typeable) is probably a little too complicated to serve as a good example.

So I'm content with having a simple convert function.

Comment thread exercises/resistor-colors/examples/success-standard/package.yaml Outdated
Comment thread exercises/resistor-colors/test/Tests.hs Outdated
@emcoding
Copy link
Copy Markdown

I noticed that the Readme here is an older version of the Readme we actually implemented.

@emcoding
Copy link
Copy Markdown

emcoding commented Mar 11, 2019

@tejasbubane Can you regenerate the readme with
configlet regenerate -o resistor-colors? what Erik posted below?

(thanks/cc @ErikSchierboom )

@ErikSchierboom
Copy link
Copy Markdown
Member

ErikSchierboom commented Mar 11, 2019

Oh, my mistake. That command should actually be: ./bin/configlet generate . -o resistor-colors
And that assumes that there is an up-to-date problem-specifications repository located at ../problem-specifications

@tejasbubane tejasbubane force-pushed the implement-resistor-colors-exercise branch 2 times, most recently from b9ad557 to 8c19291 Compare March 11, 2019 11:30
@tejasbubane
Copy link
Copy Markdown
Member Author

tejasbubane commented Mar 11, 2019

Regarding the configlet segfault issue - Sorry that was my bad. I had an old version (which was segfaulting) lying in my $PATH and that was picked up instead of bin/configlet:

configlet=$(which configlet)
if [ ! -x $configlet ]; then
configlet=bin/configlet
fi

I have updated the README and the ensure script works fine now.

@tejasbubane tejasbubane force-pushed the implement-resistor-colors-exercise branch from 8c19291 to f8bb197 Compare March 11, 2019 11:57
Copy link
Copy Markdown
Contributor

@sshine sshine left a comment

Choose a reason for hiding this comment

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

Travis CI reports a type error in the stub:

/home/travis/build/exercism/haskell/build/resistor-colors/stub/test/Tests.hs:20:29: error:
    • Couldn't match type ‘Color’ with ‘[Char]’
      Expected type: [String]
        Actual type: [Color]
    • In the first argument of ‘value’, namely ‘input’
      In the first argument of ‘shouldBe’, namely ‘value input’
      In the expression: value input `shouldBe` expected
   |
20 |         assertion   = value input `shouldBe` expected
   |                             ^^^^^

Other than that, I have no further comments before hitting approve.

Comment thread exercises/resistor-colors/src/ResistorColors.hs Outdated
@tejasbubane tejasbubane force-pushed the implement-resistor-colors-exercise branch from f8bb197 to 1e7fc58 Compare March 11, 2019 13:17
@sshine sshine merged commit 0812c41 into exercism:master Mar 12, 2019
@tejasbubane tejasbubane deleted the implement-resistor-colors-exercise branch March 12, 2019 09:46
sshine added a commit that referenced this pull request Jan 14, 2020
resistor-color-duo was added in #808.

resistor-color-trio was added in #869.

Yet there is no resistor-color. I argue that there shouldn't be.

A reasonable solution might look like

```haskell
data Color = ...
  deriving (Eq, Enum, Bounded)

colorCode :: Color -> Int
colorCode = fromEnum

colors :: [Color]
colors = [minBound..]
```

which makes it approximately as simple as the gigasecond exercise which
was marked as deprecated in #280 (decision made in #230). Still, if it
is simply missing, it will appear as an exercise that is potentially
implementable on https://tracks.exercism.io/haskell/master/unimplemented

Since a question was raised about this exercise once, marking it as
foregone will limit further requests made in vain.
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.

5 participants