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 new concept json and concept exercise githup-api #694

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

jiegillet
Copy link
Contributor

@jiegillet jiegillet commented May 20, 2024

Closes #538

@ceddlyburge, @mpizenberg I would love to have notes from both of you on this one, since it's one of those tricky topics for beginners. I'm not convinced I took the best approach, so any feedback is welcome. There is no hurry, let's take our time and get it right.

For the concept exercise, as we discussed on the issue, I took the approach of writing decoders for a real-life GitHub API. Not all fields are included of course, but the ones that are mentioned and their specs are real, which felt nice.

For the concept introduction, I tool the approach of decoding GeoJSON, which got me excited at first because it's another real use case, but now I wonder if it wasn't too complex.

Configlet on CI fails, which is expected, it will keep failing until we finalize the intro and I copy it in the exercise.

@jiegillet jiegillet marked this pull request as ready for review May 26, 2024 12:58
@mpizenberg
Copy link
Member

The learning objectives in the design document look good, although as you suggested it might be a lot? I suppose this is pretty far in the curriculum so probably it doesn’t matter as much and it’s good to have some meaty exercises?

Just to clarify, because I haven’t looked at the platform for quite some time. I should review the concept about.md which will be the first thing students will read? (Or at least a simplified minimized version of that). And then there is the exercise instructions.md ?

@jiegillet
Copy link
Contributor Author

The learning objectives in the design document look good, although as you suggested it might be a lot? I suppose this is pretty far in the curriculum so probably it doesn’t matter as much and it’s good to have some meaty exercises?

That's what I'm leaning towards. It want people to be able to use JSON decoders after going through this, so we should cover whatever will get them 80% of the way there.
The parsing concept is also similarly beefy.

Just to clarify, because I haven’t looked at the platform for quite some time. I should review the concept about.md which will be the first thing students will read? (Or at least a simplified minimized version of that). And then there is the exercise instructions.md ?

Yes, that's right, the first thing they will see is the instructions (I'm not really planning on minimizing too much other than the last section, because everything is necessary for solving the exercise), then the exercise instructions, along with the hints if they choose to read them.

@jiegillet
Copy link
Contributor Author

@mpizenberg @ceddlyburge
I know I said we can take our time, but let's still move forward :)
If you have any concerns concerning the approach, we can revisit it, and if you don't, the concept and exercise are ready for review.

@ceddlyburge
Copy link
Contributor

Hi @jiegillet , what sort of feedback are you looking for? Should I review this with the plan to approve it? Or do you want more general comments on the approach taken and things like that?

@jiegillet
Copy link
Contributor Author

I was thinking a more general one first, since JSON decoders is a pain point for beginners, it's worth thinking about the approach. It's been a while since I opened the PR though, I don't remember the details.

If it's too much work, review with the intent of approving is also fine, because it's a bit wasteful to just leave it unreviewed forever.

Copy link
Contributor

@ceddlyburge ceddlyburge left a comment

Choose a reason for hiding this comment

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

Hi Jie, overall I think this looks great.

about.md is very long, and I think I would not have been able to understand it when I was new to Elm and new to JSON decoding / parsing, its very different to how things are done in most non functional languages.

You have described a lot of the decoders in about.md, which is good, but each decoder doesn't have that much description. I could understand it, but I had to read it pretty carefully, and I already know how to do it! I think it might be better to add more description and examples to some of the simpler decoders, but with examples of combining things together, to really bed in the ideas / concepts. Then just mention the other decoders and what they do, but leave the student to go and look them up when they need to.

Also, are there any common gotchas for new students? I can half remember some frustrations when I was learning, but only half!

A quick search on slack shows up:

Not understanding Decode.map
https://elmlang.slack.com/archives/C0CJ3SBBM/p1723375470463649

Getting confused about json property names and elm record names
https://elmlang.slack.com/archives/C0CJ3SBBM/p1720733981844319

Cheers, Cedd

Decode.decodeString Decode.float "3.14"
--> Ok 3.14

Decode.decodeString (Decode.null AnyValue) "null"
Copy link
Contributor

Choose a reason for hiding this comment

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

This one might want a description. It matches a "null" and returns a "AnyValue", which doesn't fit that well with a concept of "decoding". Its more akin to parsing or matching really. Maybe the bit above could describe it a bit more fully?

Decode.decodeString (Decode.list Decode.int) "[1, 2, 3]"
--> Ok [1, 2, 3]

Decode.decodeString (Decode.list Decode.int) """[1, 2, 3, "not an int"]"""
Copy link
Contributor

Choose a reason for hiding this comment

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

Probably worth being consistent with """ and ", otherwise people might trying to read something in to it.

--> Err ...
```

Note that `Decode.list : Decoder a -> Decoder (List a)` and `Decode.dict : Decoder a -> Decoder (Dict String a)` each expect a decoder as argument, which is expected to decode every single element elements of the data structure.
Copy link
Contributor

Choose a reason for hiding this comment

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

Might be worth talking about combining decoders here? People get excited about combinators, and they are powerful, but they aren't as mysterious as people think!

decodeFeatureId : Decoder String
decodeFeatureId =
Decode.oneOf
[ Decode.string
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this make sense? I think the string will always match, so the int or float options won't be used ...

--> Ok "seventeen"

Decode.decodeString decodeFeatureId """{"id": 17}"""
--> Ok "17"
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be Ok 17

[encode]: https://package.elm-lang.org/packages/elm/json/latest/Json-Encode
[geojson]: https://geojson.org/
[pipeline]: https://package.elm-lang.org/packages/NoRedInk/elm-json-decode-pipeline/latest/Json.Decode.Pipeline
[extra]: https://package.elm-lang.org/packages/elm-community/json-extra/latest/Json.Decode.Extra
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a great blog post about this kind of thing, and why static type systems are probably better than dynamic ones, even with the extra difficulty of the json encodding / decoding and similar things
https://lexi-lambda.github.io/blog/2020/01/19/no-dynamic-type-systems-are-not-inherently-more-open/
It might be worth a link, and a bit of chat in this document. It does take longer encoding and decoding json in elm, and it is often harder, especially when you first start, but there is definitely a pay off as well.


Define `decodeName` so that it can decode a string in the `name` field if there is one.
No name? No problem, simply return `Nothing`.
This decoder should never fail.
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this misleading? What if the name has a number property?

--> }
```

## 4. I'm a strong independent comment
Copy link
Contributor

Choose a reason for hiding this comment

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

This section seems doesn't seem to teach anything new, I think its all been covered in '## 2. What's my name again?'

In which case we could remove it to make the exercise quicker to complete.

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.

Concept: JSON Decoders
3 participants