tree-building: port

[guygastineau]

This doesn't point to a canonical-data.json file, because none exists: problem-specifications

I stole all of the tests from the go track. I think it would be good to have tests that verify it can handle more levels of recursion. go tests

I think I probably should have used Either instead of Maybe, but now we can have a discussion about what would be best.

This is a refactoring exercise. I might have made the gross version too gross, but it definitely can be refactored :rofl:

I think that my correct solution could probably do with some refactoring too ;)

PS. I meant to send this up here last week, but I've been distracted by life itself. It feels good to get moving on this again ;)

[guygastineau]

So, I think the gross solution that is supposed to be shipped with the exercise for the student to refactor got too many linting suggestions and made the Travis build fail.

[guygastineau]

Actually a bunch of the linting warnings came from my clean solution too. I'll work on those.

@sshine do you know if that is why it failed?

[guygastineau]

Okay, so I got rid of the linting errors for the solution in examples/success-standard/src, but we should be expecting warnings from the sub-optimal solution given to the students to refactor.

This appears to be why it is failing unless I am misreading the report from Travis. Is there someway to exempt files that are for refactoring exercises?

[guygastineau]

I see that:

 hlint ${TRAVIS_BUILD_DIR} # Run `hlint` on the entire repository.

returns 1 as it's exit code if any warnings are given.

That seems like desirable behavior in general.

A number of the warnings I put in there intentionally, because they were redundant (to give some easy fixes during refactoring).

I suppose I could fix all of those problems. It would still all be in one giant nasty function.

Anyway, I will wait until I hear back from one of the core maintainers, before I decide on my next course of action here ;)

[guygastineau]

So I just discovered that I can disable Hlint for that single file.

If you all like the grotesqueness of the file that will be sent for refactoring, then I suppose that is one way to solve the issue.

[sshine]

I see that:

hlint ${TRAVIS_BUILD_DIR} # Run `hlint` on the entire repository.

returns 1 as it's exit code if any warnings are given. That seems like desirable behavior in general. A number of the warnings I put in there intentionally, because they were redundant (to give some easy fixes during refactoring).

You're quite right, with refactoring exercises we don't want hlint to fail in CI because of intentional mistakes. It should be possible to add a .hlint.yaml containing ignore directives:

- ignore: { within: [ TreeBuilding ] }

But I'm not sure how to distinguish the TreeBuilding of the stub with the TreeBuilding of the example solution. It's not possible to specify the directory in which to ignore a module, so perhaps we should look into replacing a repository-wide hlint . with local ones. I'm not really sure, though.

[guygastineau]

It appears like we can put the ignore rules in the stub file as ANN pragmas for it to apply specifically to that file. hlint manual Unfortunately there are no anchors on the site for # linking, but the info is near the bottom.

The only issue I see is that is would introduce a lot of clutter in the file, and we would probably need to add nformation in the README.md telling the student to remove these pragmas as they fix the problems.

[guygastineau]

I think I responded in some way to each thing 😆

I have to move a 1400 lb millingachine today, so I won't get any more work up until this evening in US/EST.

@sshine as always thank you very much for your constructive guidance 😁

[guygastineau]

Just an update. I've been super busy recently, and I will be through Saturday. Should get some time early next week to finish up on this project :smile:

[sshine]

It appears like we can put the ignore rules in the stub file as ANN pragmas for it to apply specifically to that file.

I'm sorry that I didn't address this in my research: Yes, we could do that, but then we would distribute the pragma to the student, which would cause HLint to be disabled on their end, which we really don't want. So ideally we should find a workaround HLint not having an --ignore-file or --ignore-directory feature, which most likely will complicate the very neat hlint . at the root.

The long-term benefit we get from working out this part is that we make way for handing out stubs with warnings in them for other refactoring exercises. I think having obvious errors that HLint catches is a good basis for refactoring, since it encourages the use of HLint.

[guygastineau]

Okay, those are some compelling reasons to get this figured out the right way regarding hlint 😁

[guygastineau]

I'll finally have some time for this tonight !

I looked at Data.Tree and it is exactly what I need.

[sshine]

A last thought -- sorry for spamming you -- if we're giving a

data Thread = Leaf Id | Branch Id Children

in the stub, but we'd like people to use

import Data.Tree (Tree)
type Thread = Tree Id

then how can we express test cases abstractly? We would need to hide the concrete constructors (Leaf, Branch for the stub, Node for the example) and instead export ~~functions for extracting the posts of a thread~~ an abstract constructor, i.e.

module ForumThread (Record, Thread, fromList, thread) where

...

-- In the stub
thread :: Id -> [Thread] -> Thread
thread recordId [] = Leaf recordId
thread recordId children = Branch recordId children

-- In the example
thread :: Id -> [Thread] -> Thread
thread = Node

Then we can express expected trees in the test file using e.g.

Just (thread 0 [ thread 1 [], thread 2 [] ])

regardless of internal representation.

Then it can become an objective to change the internal representation to Data.Tree without affecting the test suite.

[guygastineau]

@sshine that is amazing!

Thank you for sharing that strategy with me 😍

[sshine]

@guygastineau: I feel that I've left you a bit of a mess wrt. feedback.

I'll try and summarize my proposal of what we need to do from here:

• Rename the stub module into ForumThread
• Rename the main type the module exports into ForumThread
• Rename the main function that the module exports into fromList
• Create and use an abstract thread constructor in the stub, test and example, so students can refactor data ForumThread = ... into (new)type ForumThread = Tree Id without having to change the tests.
• Fix imports and exports in the stub, test and example to something like Record(..), Thread, thread, fromList.
• Make a decision about some of the negative cases, as I've outlined above.

In this gist I've updated the stub file and the tests so they conform to these proposals, and the only thing I've left commented out are the negative cases.

As for your example solution: Feel free to stop refactoring whenever you feel like it. The important part here is that it's a learning experience for you and that it behaves the same as the stub file. We can always take iterations on improving it further for your sake after merging the important parts.

[guygastineau]

@sshine thank you for your detailed work.

I will see how far I can take this with your guidance ;)

I'm sorry I've left this PR up without progress for so long. A new project for a startup is taking a lot of time from my days.

[guygastineau]

Hey @sshine That is a really good Gist! I appreciate all of your guidance here ;)

I am still swamped with real work projects for a start up right now. I could probably implement these changes over the next day or so, but if you are willing simply to change this to the better version I would be grateful.

I am sorry that I have left this PR hanging for so long on your track.

[sshine]

@guygastineau: We've had PRs open for a year, it really is no inconvenience. I'll look at it if you prefer to not continue with it. Thanks for all the effort so far!

[guygastineau]

@sshine your generosity of spirit is only matched by your expertise.

Though it might take me til Monday to have it in good shape I suppose I should do it (it will help help me be a better Haskeller). Thank you for dissuading my anxieties about longer running PR's.

[guygastineau]

Thank you for the review @petertseng

I can try to do some work on this this week, but I am still utterly swamped.

I have taken a huge interest in Scheme, and I am trying to help revitalize that6 very ignored track with another enthusiast right now ;)

I have done most of the work for the bob-andfriends` exercise idea, so I expect to get that up here soon.

Even though I said I should finish this PR for my benefit, I honestly would be grateful to anyone who wanted to finish this tree building port up.

[sshine]

would be grateful to anyone who wanted to finish this tree building port

You already did the heavy lifting.

I'm playing with stateful tree generators for this one, so it may take a couple of days.

[guygastineau]

@sshine cool. I am excited to see your work. I imagine I will learn a lot from reading it 🤓

In the mean time I will get the bob-and-friends PR up.

[sshine]

I've gotten seriously frustrated at QuickCheck-GenT. It's old, so it seems you need to run the stateful generator using something like

instance Arbitrary Thread where
  arbitrary = (`evalState` state) <$> runGenT (sized threadGen)

I ditched the thing when I couldn't easily call on deriving (..., MonadState ..., MonadGen ...) having to explicitly lift non-GenT features from QuickCheck's Gen like shuffle. Meh. So I'm redoing it with Hedgehog.Gen that is naturally implemented mtl-style. Hedgehog deals with generating recursive structures in a less primitive way, so I want to learn to do that part right.

[guygastineau]

Well, this sure has been open for along time ;)

I have learned a lot since I tried porting this exercise. I think I will give it another go now.

[sshine]

@guygastineau: Cool :)

I think I learned something about how to not give feedback doing this one, haha. I think if you regard all of the comments above as one big brainstorm and do the exercise without constraints, maybe it will be less critical. Feel free to decide whether you want to close this one and open a new one, or just work from here and not let the comments made so far disturb your creative process too much.

[sshine]

@guygastineau: Upon revisiting the comment thread in this PR, I see this comment containing a link to a proposed copy of the handed-out solution and a test suite that is compatible with it. You would only need to make an example solution that you like and conforms to this test suite (which was made flexible wrt. internal representation, so that this could become a part of the task).

[guygastineau]

@sshine hahaha. Well, I think your feedback was useful even though some of it lost me at the time ☺️

I got interested in this again, because I thought I might be able to use recursion schemes in the example. I'll see if it is practical with the internal representation. Still, my point is that your advice was good, but it was over my head at the time. The above brainstorming session makes much more sense now that I have gone on to learn and build more Haskell projects in the wild that do actual work. My ongoing quest to learn intuitionistic type theory is also working out (though it is hard), that and some explorations in compilers has made a lot of things clearer for me ☺️

I will need to see if I can get tools here to work for me. Last time (for some reason I can't understand) I did not reach out about the testing tools not working. Is stack still set up to use some sandbox directory to run all of the tests?

IDK, I will re-familiarize myself with the process of testing and contributing to this project. If I encounter similar issues using the repo's utilities I will reach out for help with that this time.