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.

Sign up for GitHub

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

Jump to bottom

Updating the Nucleotide Count exercise to return Either #165

Merged
merged 1 commit into from
Jul 2, 2016
Merged

Updating the Nucleotide Count exercise to return Either #165

merged 1 commit into from
Jul 2, 2016

Conversation

samjonester
Copy link
Contributor

@samjonester samjonester commented Jun 29, 2016 •
edited
Loading

The resulting exercise is more idiomatic to Haskell. The Either will provide a useful error message to end users of the functions.

Closes #157

@samjonester samjonester changed the title Updating the Nucleotide Count exercise to return Either. The resulting exercise is more idiomatic to Haskell. The Either will provide a useful error message to end users of the functions. Updating the Nucleotide Count exercise to return Either Jun 29, 2016
@samjonester samjonester changed the title Updating the Nucleotide Count exercise to return Either Updating the Nucleotide Count exercise to return Either Jun 29, 2016
@samjonester
Copy link
Contributor Author

@rbasso I borrowed your implementation for the example implementation. However, it requires the multiset package. I'm not sure whether it should be added to the build, or if I should implement the example using only the standard lib. I'm fine with either approach, but which do you think I should take?

rbasso
rbasso reviewed Jun 29, 2016
View reviewed changes
exercises/nucleotide-count/example.hs
nucleotideCounts xs = fromDistinctAscList <$> mapM count' "ACGT"
where
count' x = (\c -> (x, c)) <$> occur' x
occur' x = occur x . fromList <$> mapM valid xs
Copy link
Contributor

@rbasso rbasso Jun 29, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

The use of <$> will probably fail if base < 4.8, because it was only in Control.Applicative before that.

It's a little tricky to use it in a compatible way, because if you import Control.Applicative ((<$>)) it would raise a warning (#122) on newer versions, and we intentionally fail all tests with warnings on Travis-CI.

@rbasso
Copy link
Contributor

rbasso commented Jun 30, 2016 •
edited
Loading

Thanks for making this pull request, @samjonester. 😄

Right now we don't have too much flexibility about the packages we can use.
To add a package, we have to update:

  • README.md
  • docs/INSTALLATION.md
  • _test/bootstrap.sh
  • _test/dependencies.txt

We expect to simplify this situation in the future with #162 , but now this is what we have.

Also, considering that it's not easy for beginners using cabal to deal with new dependencies, I would avoid adding new packages for now unless they are really needed.

Recently, I updated the README.md with instructions on how to use stack to test an exercise locally. Could you please take a look at that, as I believe it would throw an error with GHC 7.8.4.

@rbasso rbasso mentioned this pull request Jun 30, 2016
Closed
@samjonester
Changing return type of Nucleotide Count to Either
8d20130 
By using an `Either` instead of throwing an error, the resulting exercise
is more idiomatic to Haskell. The `Either` will provide a useful error
message to end users of the functions.

Closes exercism#157
@samjonester
Copy link
Contributor Author

I've subverted the problem with prelude with import qualified Control.Applicative as A, and using A.<$> + A.<*>.

Also, I've removed the dependency on Data.Multiset by rolling my own naive implementation of the two functions were being used.

@rbasso I was unable to get _test/stackilize to run.

$ _test/stackalize --resolver lts-6.4 exercises/nucleotide-count
_test/stackalize: No resolver specified.

@rbasso
Copy link
Contributor

rbasso commented Jul 2, 2016 •
edited
Loading

Thanks, @samjonester !

About the stackalize problem, it is a bash script I improvised to run on Travis-CI. It was tested only on my machine and on Travis, so you are probably the first alpha-tester. 😁

If you have the time, could you open an issue with further details? I would like to fix it, as it is the only practical way we have to test problems locally with restricted dependencies.

It would be good to know:

  • What is your OS?
  • What bash version are you running? bash --version
  • What getopt version are you running? getopt --version

I guess the problem is the lack of GNU's getopt, that usually comes from package util-linux.

This link may be useful if you are running OS X:

If you're running Linux and got this error, I probably did something really wrong. 😀

@rbasso rbasso merged commit f5111d1 into exercism:master Jul 2, 2016
@cjlarose cjlarose mentioned this pull request Jul 6, 2016
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@samjonester @rbasso