Skip to content

clock: Improve hint #910

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

Merged
merged 6 commits into from
Apr 22, 2020
Merged

clock: Improve hint #910

merged 6 commits into from
Apr 22, 2020

Conversation

kevin-meyers
Copy link
Contributor

Closes #908
I added a few things:

  • Updated implementation lines to specifiy the type of clock, and that you can add or subtract both hours and minutes from it.
  • Added descriptions for the remaining 2 functions: toString and fromHourMin
  • Added a resource from learn you a haskell for creating and using custom types
  • Added restrictions to not use the inbuilt data.dates package. I followed the format of other problems.

@kevin-meyers
Copy link
Contributor Author

I am new to contributing both to exercism and in general, can someone stop by and let me know why editing the README is causing failing tests?

Copy link
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.

Hi Kevin, and welcome to open source!

Thanks for your PR, and sorry for not getting back to you between #908 and #910.

The CI failure you see is caused by a broken condition: The content of an exercise's README.md must be the concatenation of the canonical description of the exercise, and the track-specific hint file, .meta/hints.md for the exercise.

To preserve the condition, one could either

  1. change the canonical description upstream in the problem-descriptions repository, since this part of the description is shared among all tracks that implement the exercise, after which this change can propagate to the track in a PR like this one, or
  2. move all changes in this PR to .meta/hints.md

followed by regenerating README.md by running bin/ensure-readmes-are-updated.sh and then committing and pushing the changes.

Since exercism/problem-specifications#1560 (2019-07-24), only bugfixes have been admitted upstream, so I suggest you go with the latter and make changes to the hint only. This is not ideal because we may actually wish to change the canonical description. But if we were to do that, we'd need to either break the condition described above, or put in some organizational work to make the common repository work again.

As for the content of this PR itself, I think your comments do improve the description.

Let me know if you need any assistance with git or Travis CI for you to continue with this improvement.


My recommendation:

  • Add any changes to .meta/hints.md instead.
  • Address the [types] link. (See inline comment.)
  • Address the comment on ## Restrictions. (See inline comment.)

Comment on lines 31 to 34
If you need help getting started with Types, take a look at:
- [Making Our Own Types and Typeclasses][types]

[types]: http://learnyouahaskell.com/making-our-own-types-and-typeclasses
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this chapter is ideal for addressing this exercise.

The first half of the chapter only deals with data definitions, and about halfway through it addresses type aliases. But since this exercise is best solved with a newtype, and this chapter doesn't even mention newtype, it is perhaps more confusing than clarifying.

Having a hyperlink is good, but preferrably to something that focuses on the particular learning goals of the exercise, rather than types in general.

I would recommend either finding another source or removing this hint.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Got it, you're right! I used it when I solved the problem but I had to jump down at least halfway because I knew where what I was looking for was. If I can't find a better resource thats more concise I'll remove it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So I found a good resource https://mmhaskell.com/blog/2017/12/24/haskell-data-types-in-5-steps

I am going to replace it for now so let me know what you think. (or if its just better off removed). My motivation to adding something was driven because I didn't know how to get the value of 'hour' or something out of a clock object.

@kevin-meyers
Copy link
Contributor Author

Thank you for getting back to me @sshine ! I appreciate you breaking it down for me, I did find .meta/hints when I was digging around. But since it looked like it had the same contents in both hints and the README i ignored it.

I believe I understand correctly: Upstream the problem description that is shared between haskell/python etc, all of them may benefit from the change. But because [I assume v3 is in progress] the change is quickest made local to haskell here in the hint file.

One quick question though, what is your comment on restrictions?

@kevin-meyers
Copy link
Contributor Author

Also I wanted to let you know I appreciate that you took the time to explain the steps to me instead of just tackling it on your own, even though it wouldve taken you no time at all.

Copy link
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.

You can read about upstream being blocked in exercism/problem-specifications#1560.

One quick question though, what is your comment on restrictions?

So that comment disappeared because of a bug in GitHub, it seems.

I'll try to repeat it.

@kevin-meyers
Copy link
Contributor Author

I will have time to fix it all up either in a couple hours tonight or in the morning!

@kevin-meyers
Copy link
Contributor Author

kevin-meyers commented Apr 21, 2020

Okay @sshine so I did what I believed was right, and when I ran ./bin/ensure-readmes-are-updated.sh, it generated a large problems-specifications folder, but what was inside wasnt too interesting, and it didnt seem to have a place in the repo either... So I just confirmed that it worked and removed the problems-specifications folder.

I might need your eyes again haha!

@sshine
Copy link
Contributor

sshine commented Apr 21, 2020

@kevin-meyers: What that script does is create a copy of problem-specifications, and rebuilds README.md's based on the concatenation I described. What I personally do is move the problem-specifications git repo one level out and leave a symlink inside the exercism haskell repo. But you can just keep a copy of problem-specifications in the track repo, or delete it again.

What that does, if you inspect the script, is that it re-generates the README. So after you have changed the hint file and regenerate the README, your README will contain the hint in such a way that the CI script will regard them as having been built properly.

The part of bin/ensure-readmes-are-updated.sh that does this is:

$configlet generate . --only "$exercise" --spec-path "problem-specifications"

This assumes you have configlet installed.

@kevin-meyers
Copy link
Contributor Author

I found configlet (i didnt realize it was a part of exercism so i was scouring the internet!), and installed it. Managed to see the generated files! now I am making sure it looks correct and passes the tests.

@kevin-meyers
Copy link
Contributor Author

I put the changes in hints, and the descriptions changes in the problem specifications file. I then used the command you provided me with configlet and the path of problem-specifications to generate the README as described!

I get the feeling it will still break though but I remain optimistic.

@kevin-meyers
Copy link
Contributor Author

okay so it says its out of date, that might be because I edited the problem specifications and shouldnt have... So I feel like I can only change everything below the clock basic description. I am going to try leaving the problem specifications alone, and just "add" information above ## hints as if it were a continuation of description.

@kevin-meyers
Copy link
Contributor Author

Sorry about running the integration tests a couple times here. If this one doesn't click I will wait until you are available again. Thank you so much for your patience

Copy link
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.

Very nice. I have one suggestion and one typo fix.

@kevin-meyers wrote:

I feel like I can only change everything below the clock basic description

I feel with you.

So I wrote:

I suggest you go with the latter and make changes to the hint only.

@kevin-meyers
Copy link
Contributor Author

This has been a long and ardous journey haha! Thank you so much for taking the time to guide me through the process and philosophies behind making changes as little as sentence positioning.

I am continuing to learn haskell 💪, I look forward to contributing more in the future (hopefully with less hand holding required!)

I am a huge fan of the platform, and couldn't have asked for a better way to get in gear.

@sshine sshine changed the title Added description to clock instructions. clock: Improve hint Apr 22, 2020
@sshine sshine merged commit f1f0dd7 into exercism:master Apr 22, 2020
@kevin-meyers kevin-meyers deleted the 908-clock-is-confusing branch April 22, 2020 16:25
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.

Feedback: Exercise Clock is a little confusing
2 participants