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

adding isogram #595

Merged
merged 8 commits into from
Sep 23, 2017
Merged

adding isogram #595

merged 8 commits into from
Sep 23, 2017

Conversation

Average-user
Copy link
Member

Another Unimplemented exercise

@Average-user
Copy link
Member Author

This one is ok. right?

config.json Outdated
"unlocked_by": null,
"difficulty": 1,
"topics": [
"Data.Char"
Copy link
Member

@petertseng petertseng Sep 20, 2017

Choose a reason for hiding this comment

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

in contrast to Maybe, I don't think this:

  1. affects where we would choose to place the exercise
  2. is necessarily going to be in everyone's solutions

so I would remove it

Copy link
Member Author

Choose a reason for hiding this comment

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

right

@@ -0,0 +1,20 @@
name: isogram
version: 1.1.0
Copy link
Member

Choose a reason for hiding this comment

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

1.1.0.1

@@ -0,0 +1,64 @@
{-# OPTIONS_GHC -fno-warn-type-defaults #-}
Copy link
Member

Choose a reason for hiding this comment

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

given that #144 tells us this line helps for exercises with numbers but this is not one, I think it can be removed?

@@ -0,0 +1,6 @@
module Isogram (isIsogram) where

import Data.Char
Copy link
Member

Choose a reason for hiding this comment

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

since

  1. Not every solution wants or needs this
  2. I wouldn't want to encourage the habit of importing everything rather than the specific list of functions desired

I would remove this line

Copy link
Member

@petertseng petertseng left a comment

Choose a reason for hiding this comment

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

OK, I don't think there's anything big to change, so it will be merged no more than 48 hours. There are two small things. I was going to do them myself. You can if you want.


## Source

see more at [wikipedia page of isograms]("https://en.wikipedia.org/wiki/Isogram")
Copy link
Member

Choose a reason for hiding this comment

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

as you can see in https://github.com/Average-user/haskell/blob/d1556eadc748688089e3261700ab9678f2caf245/exercises/isogram/README.md, the quotes mean it is not a link.

I'm going to regenerate this README anyway so it doesn't matter whether you fix this, but don't put quotes in the brackets

Copy link
Member Author

Choose a reason for hiding this comment

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

ok, so the next time i make PR like this, do i even have to put the README.md?

Copy link
Member

Choose a reason for hiding this comment

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

I guess not! I will just do it. However, it would be good if you just put a README.md that just says "please regenerate this for me" so that I don't forget to!

Copy link
Member Author

Choose a reason for hiding this comment

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

ok, also, dont merge yet, i want to implement something

@@ -0,0 +1,5 @@
module Isogram (isIsogram) where

Copy link
Member

Choose a reason for hiding this comment

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

I would remove one newline here so there is only one between module and isIsogram, not two.

@Average-user
Copy link
Member Author

Now you can merge if you want

@Average-user
Copy link
Member Author

How can i do if i want to do another PR before this one gets merge?

@petertseng
Copy link
Member

How can i do if i want to do another PR before this one gets merge?

Make a branch.

@Average-user
Copy link
Member Author

But Github will still catch the differences with the (in this case) Isogram commits

@petertseng
Copy link
Member

But Github will still catch the differences with the (in this case) Isogram commits

Github shows all commits that are not on the target branch (Exercism's master branch) but are on the PR branch (your new branch). The Isogram commits should not be on your new branch. Achieve that using whatever means you prefer. Ask if you want a suggestion of a preferred means.

@Average-user
Copy link
Member Author

Thanks, i'll take that suggestion you offer .

@petertseng
Copy link
Member

Case 1: You do not already have a branch.

You decide on a branch name, and then you git checkout -b your_new_branch_name 7dc8cf2f0403857a79d13c4916d4b26f99af15e4

(why, because 7dc8cf2f0403857a79d13c4916d4b26f99af15e4 is the latest commit shown on https://github.com/exercism/haskell/commits/master )

Case 2: You already have a branch.

You git checkout your_existing_branch_name if you are not already on that branch, then you git rebase -i 7dc8cf2f0403857a79d13c4916d4b26f99af15e4, then you delete all lines that are related to Isogram.

@Average-user
Copy link
Member Author

Instead of this key (or whatever) 7dc8cf2f0403857a79d13c4916d4b26f99af15e4 can I use the link https://github.com/exercism/haskell?

@petertseng
Copy link
Member

Instead of this key (or whatever) 7dc8cf2f0403857a79d13c4916d4b26f99af15e4 can I use the link https://github.com/exercism/haskell?

If the repository https://github.com/exercism/haskell is a remote on your local copy of the git repo, you may instead use your_name_for_the_exercism_remote/master instead of 7dc8cf2f0403857a79d13c4916d4b26f99af15e4.

Copy link
Member

@petertseng petertseng left a comment

Choose a reason for hiding this comment

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

far as I can tell, we are ready to go.

@petertseng petertseng merged commit 53a2299 into exercism:master Sep 23, 2017
@petertseng
Copy link
Member

Thank you for implementing this exercise!

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