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 practice exercise rest-api #621

Merged
merged 3 commits into from
Oct 27, 2023
Merged

Add practice exercise rest-api #621

merged 3 commits into from
Oct 27, 2023

Conversation

jiegillet
Copy link
Contributor

@jiegillet jiegillet commented Oct 23, 2023

I'm now thinking about the json concept, and this is the most suitable practice exercise for it, maybe even the suitable one? I'm sure we could engineer some other to use JSON as raw data, I don't know.

I quite like my solution, it's pretty clean, but I don't really know about the general API. The description in problem-specs itself is pretty vague about it. Right now what I have is

  case RestApi.buildDatabase "{\"users\":[]}" of
      Err error ->
          Expect.fail ("== Database payload could not be parsed ==\n" ++ Decode.errorToString error)

      Ok database ->
          RestApi.post database "/add" "{\"user\":\"Adam\"}"
              |> Expect.equal "{\"name\":\"Adam\",\"owes\":{},\"owed_by\":{},\"balance\":0}"

I'm feeding the raw JSON into buildDatabase in one step, check if that worked, and then use post. I did this so that the parsing for the database could be done separately.
Ideally the "server" would keep the database in an internal state, but it can't work in Elm.
Anyway, I'm happy for some feedback on that API.

This will require an analyzer check to encourage students to use Json.Decode and Json.Encode, because technically they could do it without.

"slug": "rest-api",
"name": "REST API",
"uuid": "98b75220-b8bb-488b-9339-a3972f040c21",
"practices": [],
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This will practice "json" when it exists

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.

Hey Jie, you really have found a lot of enthusiasm, you're creating PRs faster than I can review them!
This exercise looks good, but is complicated. The complexity around encoding and decoding JSON is what we want the student to tackle, but there is also a lot of accidental complexity around understanding the exercise and what is required, and I think we could probably improve that. Probably by adding a bit more scaffolding to the initial code, or maybe by adding comments / more information in the Debug.todo's.


buildDatabase : String -> Result Error Database
buildDatabase =
Decode.decodeString databaseD
Copy link
Contributor

Choose a reason for hiding this comment

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

I would probably call this databaseDecoder, but feels like a personal preference type of thing.

Dict Name User


buildDatabase : String -> Result Error Database
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 difficult exercise, and if we can't change the instructions, then I think it would be worth adding some more help in this file (there is nothing in the instructions detailing what this function should do). I'm not exactly sure how best to do this, but maybe this function could be called databaseFromJsonString or something like that? Or we could supply this function ready made and ask the student to write the decoder, and then the type signature communicates what they need to do.

Debug.todo "Please implement buildDatabase"


get : Database -> String -> Maybe String -> String
Copy link
Contributor

Choose a reason for hiding this comment

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

I think a similar thing applies here. I don't think its totally clear that the return value should be a json string encoding the "Response with payload" from the instructions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe I could require that they return a Json.Encode.Value?
Or more simply we could have a type alias JsonString = String.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah something like that sounds good. I think we also need to state what the format of the json should be, there are two options in the table in the instructions, and we want the one from the "Response with payload" one. Maybe we could even define a type alias for it, then ask for a function to encode that, and then ask to compose that function here? I'm not quite sure on the details ...

Copy link
Contributor Author

@jiegillet jiegillet Oct 26, 2023

Choose a reason for hiding this comment

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

I added the alias.
I thought that Maybe JsonString encapsulated well enough that the payload can missing, so I left it this way. The docs are also pretty good to be honest (compared to some other ones).

exercises/practice/rest-api/src/RestApi.elm Show resolved Hide resolved
@jiegillet jiegillet mentioned this pull request Oct 24, 2023
@jiegillet jiegillet merged commit b635f68 into main Oct 27, 2023
5 checks passed
@jiegillet jiegillet deleted the jie-rest-api branch October 27, 2023 00:10
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