Added twelve-days haskell exercise #628

tuxagon · 2017-11-30T05:21:09Z

Implemented the twelve-days exercise for haskell with a single example code file.

petertseng

Thanks, good stuff, there not much I would change.

I need to note a particular consideration regarding the placement of this exercise. Because of #301, other text generation exercises (beer-song, house, food-chain) provide working solutions that simply emit the entire text being asked for.

It does not seem to me like it will make sense for twelve-days to become between any of these three exercises if it does not get the same treatment, so if it does not, either place somewhere before beer song or somewhere after food chain. But if it does, then no such restriction on placement.

petertseng · 2017-11-30T08:48:42Z

exercises/twelve-days/test/Tests.hs

+               , expected = [ 
+                  "On the first day of Christmas my true love gave to me, a Partridge in a Pear Tree."
+                 ]
+               },


I understand there is a consistency to what we have going on here: In all lists, all list elements got trailing commas. In all record fields, all fields got leading commas. Do I understand correctly that this is in fact intentional (for example, decided that leading commas for list elements did not look good?). I'm curious about the story.

Personally, I prefer the elm method which is to place the commas before. This was more to just be consistent with what I saw in other haskell tests. I can update it or leave it, but I'll leave it unless explicitly requested to make the alteration.

In all lists, all list elements got trailing commas. In all record fields, all fields got leading commas.

This was more to just be consistent with what I saw in other haskell tests.

The only other Haskell test where I see a trailing comma are lens-person and pascals-triangle, BUT I may have missed some because I only grepped the specific strings }, and ], and not any other variants.

Thus:

For consistency with all other tests in this repo, let us have leading commas for the separation between different Case.

I think it will also be better to have leading commas separating the strings in expected, for the purpose of having this file be consistent with itself. Even if this isn't done, there is still a sort of consistency, though it is odd, so I could be convinced that it should stay. Your option on this second one.

petertseng · 2017-11-30T08:53:36Z

exercises/twelve-days/HINTS.md

@@ -0,0 +1,4 @@
+## Hints
+
+Try to capture the structure of the song in your code, where you build up the


ah this is interesting. I see in https://github.com/exercism/docs/blob/f7a01b294f7b3fd2a01c7c856e45450d7f4eb246/you-can-help/make-up-new-exercises.md#generic-problem-specifications there is a sentence that says...

The test cases should not require people to follow one very specific way to solve the problem

However, that bullet point does not apply here, since this is not a test case. My personal feeling is that this idea should also apply here, and that therefore I would not have this hint. Let us hear your thoughts on it.

I could remove it. I was modeling it after the fsharp tests, but I do see that this hint does not exist in the elixir implementation. I can remove it.

petertseng · 2017-11-30T08:55:06Z

exercises/twelve-days/package.yaml

@@ -0,0 +1,20 @@
+name: twelve-days
+version: 1.0.0.0


For better or worse, #522 tells me that the last version component should start at 1, so let us have 1.0.0.1

petertseng · 2017-11-30T08:56:15Z

exercises/twelve-days/README.md

+one, head over there and create an issue.  We'll do our best to help you!
+
+## Hints
+- Try to capture the structure of the song in your code, where you build up the


I remind myself that I may need to regenerate based on what happens in HINTS.md

Do you want me to remove this? Keep it?

HINTS.md

Do you want me to remove this? Keep it?

Sicne it's no longer in HINTS.md, then it needs to be removed from here too, otherwise the README doesn't match what's generated.

petertseng · 2017-11-30T08:57:31Z

config.json

+      "slug": "twelve-days",
+      "core": false,
+      "unlocked_by": null,
+      "difficulty": 4,


since this is difficulty 4, it should be placed with other difficulty 4 exercises in order, rather than after difficult 10.

This has been done

petertseng · 2017-11-30T09:05:36Z

config.json

+      "unlocked_by": null,
+      "difficulty": 4,
+      "topics": [
+        "algorithms",


If the algorithms is to be added, I will want all exercises that should have the topic to have it. Since currently none do (but I assume some should, if it exists), I would ask for either:

Add it to all exercises that should have it in a separate PR or a separate commit in this PR, then keep it in this PR.

Do not add it in this PR, but after this PR is merged submit another PR that does add it.

Do not add it in this PR, and do nothing further.

I'll remove it. I looked at the fsharp implementation's config.json file for reference. I have no attachment to that specific topic, in regards to this problem.

petertseng · 2017-11-30T09:06:00Z

config.json

+      "difficulty": 4,
+      "topics": [
+        "algorithms",
+        "text_formatting"


If the algorithms is to be added, I will want all exercises that should have the topic to have it. Since currently none do (but I assume some should, if it exists), I would ask for either:

Add it to all exercises that should have it in a separate PR or a separate commit in this PR, then keep it in this PR.

Do not add it in this PR, but after this PR is merged submit another PR that does add it.

Do not add it in this PR, and do nothing further.

same (but text_formatting instead of algorithms)

tuxagon · 2017-12-03T03:20:29Z

Are there any more changes that should be made?

petertseng · 2017-12-04T13:14:11Z

Are there any more changes that should be made?

I did not give the full information when I said "place exercise with all other exercises of length 4" since the full request was contained in #628 (review) (considering the relation of twelve-days vs all other text generation exercises). I now ask your thoughts on the relation. Whether twelve-days should be moved so that it is either the first or the last, or if there is a good reason (stating the reason) to ignore that relation.

tuxagon · 2017-12-04T15:32:36Z

I think it would be a good idea, potentially, to have a better structure for config.json, in addition to having more helpful topic-descriptors, but wouldn't that be out of scope of this PR? Having a separate PR with the sole focus on defining a structure for config.json that can always be pointed to when a PR affecting it is created. I placed it at the end for no specific reason. At the time, it just "made sense" to me. What are your thoughts?

petertseng · 2017-12-04T15:43:40Z

to have a better structure for config.json, in addition to having more helpful topic-descriptors

There are many things that can be done, such as doing the core and unlocked_by explained in exercism/discussions#175. I don't recommend doing that yet since it's not useful until https://v2.exercism.io/ is released. This comment is only to explain that that is how structure would be achieved.

but wouldn't that be out of scope of this PR

Absolutely. This PR is about adding twelve-days exercise, so this PR should only do that. For adding the exercise to make sense, it be placed in the proper place in config.json, according to whatever structure config.json has at the time it is merged.

Having a separate PR with the sole focus on defining a structure for config.json that can always be pointed to when a PR affecting it is created

Yes.

tuxagon · 2017-12-04T16:00:42Z

Does that mean, you want me to update config.json to place twelve-days by another text-generation? Or it is fine, currently, at the end of the list of difficulty-4's? Is that the only outstanding item for PR changes?

edit: I just saw the comments in the above review. I missed them before. Updates made for Test.hs and README.md

tuxagon · 2017-12-04T18:21:49Z

@petertseng i don't see what i am doing wrong. what about the README is causing the CI failure?

tuxagon · 2017-12-04T19:44:52Z

I looked through the CI code and see there is something called configlet. I'll look into this and see what I did wrong using https://github.com/exercism/configlet.git

petertseng

what about the README is causing the CI failure?

I'll look into this and see what I did wrong using https://github.com/exercism/configlet.git

That is the correct place to look. If you are unable or unwilling to run configlet generate yourself, a good place to start will be to diff the file against https://raw.githubusercontent.com/exercism/problem-specifications/master/exercises/twelve-days/description.md . I wonder if it would be useful to make Travis CI output the diff between what it expects the README to be.

update config.json to place twelve-days by another text-generation? Or it is fine, currently, at the end of the list of difficulty-4's?

I would say, one of:

If you think this exercise is easier than beer song, change its difficulty accordingly then place it before beer song.
If you think this exercise is harder than food chain, change its difficulty accordingly then place it after food chain.
Otherwise, it doesn't see to make sense that this is the only text generation exercise that doesn't start with a working solution and that it comes between the others, so add a working solution to be provided to the student, like the other text generation exercises.
Otherwise, an explanation of why it is OK to have this exercise be different from the other text-generation exercises.

Is that the only outstanding item for PR changes?

These are all items I'm aware of.

petertseng · 2017-12-04T23:24:39Z

exercises/twelve-days/test/Tests.hs

+                  "On the first day of Christmas my true love gave to me, a Partridge in a Pear Tree."
+                 ]
+               }
+         , Case { description = "second day two turtle doves"


could I ask you to check the alignment of this line and all other Case lines for me? My eyes may deceive me but I was thinking they are not aligned right now.

Interesting. Yea, I can totally do that. Somehow I managed to mis-align them 0.o

tuxagon · 2017-12-05T13:57:50Z

It says I still have changes requested. Am I missing one? Or is that not automatic when code changes?

petertseng · 2017-12-07T18:16:01Z

You are correct in your suspicion that it is not automatic. Therefore, not to fret. It will indeed go away when I next can review.

petertseng · 2017-12-12T08:05:52Z

I will review when I am able. I am sorry to say that I am currently unable.

petertseng · 2017-12-23T06:11:12Z

Hello, I haven't forgotten. It is likely that any changes that need to be done are minor and I will do them, so you can rest easy not having to check in every day wondering if changes are going to be requested. Probably sometime in the next few days. Actually looks like it's already been moved before beer song so I can't imagine what else I would need to change. In that case we are probably good.

petertseng

The day has come. As I had thought before, there are only small changes to be made and I will make them instead of troubling you to do since I've held things up long enough. First I'm rebasing on master (is requirement for this repo) then I'll apply the changes I will be making.

petertseng · 2018-01-04T04:08:11Z

config.json

+      "topics": [
+      ]
+    },
+


I'll remove this extra line (otherwise I hear rumours that there is an automatic JSON formatter in the works that would remove it, so might as well get ahead of the curve)

petertseng · 2018-01-04T04:10:56Z

exercises/twelve-days/test/Tests.hs

+               , start = 1
+               , stop = 1
+               , expected = [ 
+                  "On the first day of Christmas my true love gave to me, a Partridge in a Pear Tree."


I may like to change some alignments, two spaces in, so that the commas (were they to be present) align with the closing brace. not sure if there is a strict standard here but what I propose seems reasonable.

This reverts commit 60c028c.

petertseng

I have reviewed this PR, and it is my belief that it improves the track by providing a choice of another exercise. The possible objections:

it adds yet another text generation exercise
- counterpoint: but students don't have to do this exercise. It's merely the next one suggested; it can always be skipped
  - counter-counterpoint: but some students may not know about skipping.
- counterpoint: If the linear progression no longer is appropriate, it may be time to use the tree structure that Nextercism would bring.
It is too close to beer-song, another text generation exercise
- I admit I don't know the effects of this and whether it will make students bored. We will see how it goes and adjust its placement if we get feedback.
Possible disagreement with the decision to place it as the easiest, relatively, of the now four text generation exercises
- As above, we will see how it goes and adjust its placement if we get feedback.

So now it is time to merge.

petertseng · 2018-01-04T04:31:32Z

Let me thank you for your contribution. It seems to me that this (and an accompanying PR to add this to the Elm track) are your first contributions on GitHub (I don't know whether that means it's your first open source contributions). So, welcome!

My main hope is that the fact that I took 1+ month to accept it does not discourage you from making further contributions, whether the contributions be to Exercism or other projects of your choosing. But obviously that was my fault, not yours, due to my personal issues. If you choose to contribute to this project again, you are welcome back and I'll still be around for the time being. I of course wouldn't fault you if you instead chose to pick a one with more responsive maintainers.

Thank you once again!

tuxagon changed the title ~~✨ Added twelve-days haskell exercise~~ Added twelve-days haskell exercise Nov 30, 2017

petertseng suggested changes Nov 30, 2017

View reviewed changes

petertseng reviewed Dec 4, 2017

View reviewed changes

petertseng reviewed Jan 4, 2018

View reviewed changes

tuxagon and others added 11 commits January 3, 2018 20:13

✨ Added twelve-days haskell exercise

e829486

Updated config.json

4d7b9c1

Made uuid unique

a26bbf1

Removed warning about unhandled patterns in example

e8fba25

Made PR changes

b6a9adb

Update README.md

4dfb8e1

Update Tests.hs

5854e1f

Re-added Hints

67a1567

Revert "Re-added Hints"

52daeee

This reverts commit 60c028c.

PR changes to fix configuration and formatting

b27389d

Updated difficulty to be 3

b90ce13

petertseng force-pushed the master branch from 5d5bab3 to b90ce13 Compare January 4, 2018 04:14

petertseng added 3 commits January 3, 2018 20:16

rm trailing whitespace

f34dfb1

realign

0d3c502

config: rm extra line

fef46e0

petertseng approved these changes Jan 4, 2018

View reviewed changes

petertseng merged commit b3c8a37 into exercism:master Jan 4, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added twelve-days haskell exercise #628

Added twelve-days haskell exercise #628

tuxagon commented Nov 30, 2017

petertseng left a comment

petertseng Nov 30, 2017

tuxagon Nov 30, 2017

petertseng Dec 4, 2017

petertseng Nov 30, 2017

tuxagon Nov 30, 2017

petertseng Nov 30, 2017

petertseng Nov 30, 2017

tuxagon Nov 30, 2017

petertseng Dec 4, 2017

petertseng Nov 30, 2017

tuxagon Dec 3, 2017

petertseng Nov 30, 2017

tuxagon Nov 30, 2017

petertseng Nov 30, 2017

tuxagon commented Dec 3, 2017

petertseng commented Dec 4, 2017

tuxagon commented Dec 4, 2017

petertseng commented Dec 4, 2017 •

edited

Loading

tuxagon commented Dec 4, 2017 •

edited

Loading

tuxagon commented Dec 4, 2017

tuxagon commented Dec 4, 2017

petertseng left a comment

petertseng Dec 4, 2017

tuxagon Dec 5, 2017

tuxagon commented Dec 5, 2017

petertseng commented Dec 7, 2017 •

edited

Loading

petertseng commented Dec 12, 2017

petertseng commented Dec 23, 2017

petertseng left a comment

petertseng Jan 4, 2018

petertseng Jan 4, 2018

petertseng left a comment

petertseng commented Jan 4, 2018

		@@ -0,0 +1,4 @@
		## Hints

		Try to capture the structure of the song in your code, where you build up the

Added twelve-days haskell exercise #628

Added twelve-days haskell exercise #628

Conversation

tuxagon commented Nov 30, 2017

petertseng left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tuxagon commented Dec 3, 2017

petertseng commented Dec 4, 2017

tuxagon commented Dec 4, 2017

petertseng commented Dec 4, 2017 • edited Loading

tuxagon commented Dec 4, 2017 • edited Loading

tuxagon commented Dec 4, 2017

tuxagon commented Dec 4, 2017

petertseng left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tuxagon commented Dec 5, 2017

petertseng commented Dec 7, 2017 • edited Loading

petertseng commented Dec 12, 2017

petertseng commented Dec 23, 2017

petertseng left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

petertseng left a comment

Choose a reason for hiding this comment

petertseng commented Jan 4, 2018

petertseng commented Dec 4, 2017 •

edited

Loading

tuxagon commented Dec 4, 2017 •

edited

Loading

petertseng commented Dec 7, 2017 •

edited

Loading