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

Fix type of render in refactor-1.md #6

Merged
merged 1 commit into from
Feb 24, 2023

Conversation

slowdive-
Copy link
Contributor

@slowdive- slowdive- commented Feb 18, 2023

The first refactoring exercise mentions to change the render function to :: RenderState -> Builder to use Builder instead of String. Unless I'm missing something this should be :: BoardInfo -> RenderState -> Builder, since the String version also uses the BoardInfo. This might be potentially confusing.

Edit: + Fix more type signatures in refactor-4.md

Copy link
Owner

@lsmor lsmor left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution. Would you mind to merge from main to main? This project's git tree looks something like

----- main ---------> changes in docs ---->
  |-----> solution-mvp
     |---> snake-fury-exercise
     |---> solution-refactor-1
        |---> solution-refactor-2.1
           |---> solution-refactor-2.2
              |---> etc...

I know is a mess and I should change it, but it is the way it worked for me during the building of the challenge. When I change some documentation I can merge main in every other branch.

@slowdive- slowdive- changed the base branch from snake-fury-exercise to main February 24, 2023 16:55
@slowdive-
Copy link
Contributor Author

Sure! I also squashed the commits. Seems like I can't edit my own branch in the PR though, but my main and snake-fury-exercise point to the same commit, so it should be correct the way the PR is setup now?

@lsmor lsmor merged commit 43cc802 into lsmor:main Feb 24, 2023
@slowdive-
Copy link
Contributor Author

Thanks for merging and thanks for providing this exercise, you've clearly put a lot of work into this :)

@lsmor
Copy link
Owner

lsmor commented Feb 24, 2023

Thank you for PR!! There are many small mistakes I am not aware of :). Also if you will, provide some feedback to keep improving the challenge

@slowdive-
Copy link
Contributor Author

Yes, I wasn't sure where to provide feedback so I'll just comment here.

Exercise 4 was the first one that was new territory for me, so naturally it took me a long time to figure some of it out. One thing I was missing that would have helped me, was the solution for implementing Functor, Applicative, etc. manually for the newtype. The solution branch uses GeneralizedNewtypeDeriving to just derive them. Maybe it would be helpful if one of the files, e.g. GameState.hs, kept the manual implementations of the type classes?

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