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

Add record types concept and associated exercise #369

Merged
merged 26 commits into from
Jun 23, 2021
Merged

Conversation

ceddlyburge
Copy link
Contributor

@ceddlyburge ceddlyburge commented May 22, 2021

Add records concept and associated exercise

The configlet linter is still failing, but this has been fixed in main, so will be fixed when this is merged.

@mpizenberg
Copy link
Member

Thanks for this! It's great since I'll need it as a dependency for the exercise I'm making for Maybe. I've reviewed and made changes to the about.md docs. The main changes:

  • Cosmetics:
    • to follow exercism guideline about markdown documents I've put one sentence per line.
    • for elm code blocks, I've aligned everything to the left and tried to make code consistent between each block
    • for results in elm code blocks I'm using --> since it's the syntax expected by elm-verify-examples which is often used in packages to make sure examples in the docs are not stale.
  • Content:
    • I've added segmented each part into a subsection because it seemed to be a lot of content
    • I added a section about extensible records
    • other small rephrasing

Let me know what you think of all this.

@ceddlyburge
Copy link
Contributor Author

Thanks Matthieu, those changes look good. I should finish the rest of the PR by the weekend I think.

@ceddlyburge ceddlyburge changed the title WIP: Add record types concept and associated exercise Add record types concept and associated exercise May 27, 2021
@ceddlyburge ceddlyburge requested a review from mpizenberg May 27, 2021 07:17
@mpizenberg mpizenberg added the x:size/large Large amount of work label Jun 19, 2021
Copy link
Member

@mpizenberg mpizenberg left a comment

Choose a reason for hiding this comment

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

Hi @ceddlyburge . I've finally had some time to review again your changes. I've rebased this on a newer main commit to avoid merge conflict with list concept. I was surprised that you didn't go for TV shows for the exercise but I like the bandwagoner theme too! I have some questions and remarks about the exercise, here they are.


Are you using only the record constructors in the examples to not disclose the structure too much, or for example brevity? It is usually preferable to use the curly braces syntax to define them because the constructors are dependent on order, thus potential sources of hard-to-find bugs. So I'm not sure if it is worth trying to have shorter code examples if it gives the wrong habits to students. What do you think?

For the replaceCoach function, I'd reverse the order of arguments. It's generally good to end arguments with the same type that is transformed since it improves readabily in pipes, so Coach -> Team -> Team.

Do you think the isSameTeam task is worth it? Equality in Elm works the same everywhere, and there is no concept of referential equality (except for Html lazy but that's a very particular edge case), and there is only one operator == so it's not like they could get it wrong.
I'm wondering if the small paragraph in the introduction is enough.

I feel the exercise is missing a "constructor" function. By that I mean a function that would make the student write the { field = ..., ... } entirely. I feel like a good function to do that would be to provide the arguments in the wrong order, compared to the auto-generated constructor. Something like this where Stats and Coach are reversed, preventing the student to use the Team constructor.

createTeam : String -> Stats -> Coach -> Team
createTeam name stats coach =

And at the beginning, in the instructions to define the model, we should add "with the following fields, in the following order". Then the first tests should check that the order is respected. And Same for the other type aliases.

test "Coach type alias is correctly defined" <|
    \_ ->
        Coach "Steve Kerr" True
            |> Expect.equal
                { name = "Steve Kerr"
                , formerPlayer = True
                }

Finally, regarding the rootForTeam function, if we want to make students use the extensible record syntax, we should add a test with a record that isn't a Team.

@ceddlyburge
Copy link
Contributor Author

I was surprised that you didn't go for TV shows for the exercise but I like the bandwagoner theme too! I have some questions and remarks about the exercise, here they are.

I have been trying to pick the low hanging fruit, and copy exercises from other languages. I didn't come across the TV shows one when I was looking, which track is this on (I looked at quite a few function type tracks, such as F#, Scala, Haskell, Clojure, Rust etc)

@ceddlyburge
Copy link
Contributor Author

Are you using only the record constructors in the examples to not disclose the structure too much, or for example brevity? It is usually preferable to use the curly braces syntax to define them because the constructors are dependent on order, thus potential sources of hard-to-find bugs. So I'm not sure if it is worth trying to have shorter code examples if it gives the wrong habits to students. What do you think?

This is a fair point, although I often use the constructors, partly because it communicates the type you are creating. It's true that if you have multiple properties of the same type it would be easy to mix them up though. I'll commit some changes.

@ceddlyburge
Copy link
Contributor Author

For the replaceCoach function, I'd reverse the order of arguments. It's generally good to end arguments with the same type that is transformed since it improves readabily in pipes, so Coach -> Team -> Team.

Fair point, I'll commit this change now

@ceddlyburge
Copy link
Contributor Author

Do you think the isSameTeam task is worth it? Equality in Elm works the same everywhere, and there is no concept of referential equality (except for Html lazy but that's a very particular edge case), and there is only one operator == so it's not like they could get it wrong.
I'm wondering if the small paragraph in the introduction is enough.

Yep, fair enough, I'll remove this

@ceddlyburge
Copy link
Contributor Author

And at the beginning, in the instructions to define the model, we should add "with the following fields, in the following order". Then the first tests should check that the order is respected. And Same for the other type aliases.

Good point, I've done this.

@ceddlyburge
Copy link
Contributor Author

I feel the exercise is missing a "constructor" function. By that I mean a function that would make the student write the { field = ..., ... } entirely. I feel like a good function to do that would be to provide the arguments in the wrong order, compared to the auto-generated constructor. Something like this where Stats and Coach are reversed, preventing the student to use the Team constructor.
Ok, I've added that

@ceddlyburge
Copy link
Contributor Author

Finally, regarding the rootForTeam function, if we want to make students use the extensible record syntax, we should add a test with a record that isn't a Team.
Good point, I've added a test

@ceddlyburge
Copy link
Contributor Author

I think that is all the feedback for this PR addressed now, can you give it another look? Thanks, Cedd

Copy link
Member

@mpizenberg mpizenberg left a comment

Choose a reason for hiding this comment

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

Looks great to me! one more concept ;)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
x:size/large Large amount of work
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants