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

New practice exercise rest-api #831

Merged
merged 3 commits into from
Jul 16, 2021

Conversation

jiegillet
Copy link
Contributor

Last one!
This one might need a bit of discussion as well, though. I added a design.md with some quick points.

I know we discussed that we shouldn't use a JSON strings, but the specs insisted, and after reflection I tend to agree. Manipulating JSON is important. Users may not be able to do it on the online platform, but they can do it on the CLI and request mentoring if need be.

I user :ets in my example implementation, more to learn it than anything else, but I'm not sure whether or not we should push this. The code can remain clean with maps and such. Also I used Agent because it's a simple process.

@neenjaw neenjaw requested a review from angelikatyborska July 10, 2021 11:53
@angelikatyborska
Copy link
Member

I'm really torn about this exercise. I need a few more days to think about it.

@jiegillet
Copy link
Contributor Author

Take your time, it's not going anywhere and it's not like it's blocking more exercises :)

Of course, feel free to share your concerns so we can discuss them here if you like.

@angelikatyborska
Copy link
Member

My primary concern is the JSON parsing. I found 5 other tracks that implemented this exercise: tcl, java, f#, c#, and python. 4 of those (java, f#, c#, python) use a JSON parsing library to solve it. The last one, tcl, doesn't even parse JSON and the input seems to be the language's built in dictionary data structure.

That means, that if we actually told students to parse JSONS on their own, we would be the only track. If we used maps instead of string, we would be only one of 2 tracks who implemented this exercise that way.

I feel uncomfortable with both options, but with the first one much more. I don't want to be the pioneer on Exercism that decides as the first one to make students parse JSONs... I don't know if that's a good idea.

@NobbZ
Copy link
Member

NobbZ commented Jul 14, 2021

As I've already pointed out in the slack, passing in something different than strings that the student has to parse on their own or through a library would be violating the exercises intend.

https://github.com/exercism/problem-specifications/blob/main/exercises/rest-api/canonical-data.json#L7-L9

Under this circumstances the exercise should be a "do not implement" on any track that doesn't have either a decent JSON parser in its stdlib or a defacto standard in the ecosystem, that can also be easily used within the testrunner.

At the same time, the requirement of the exercise doesn't really make sense for me.

If its just delegating to libraries or stdlib functions anyway, which get from a string to a native representation, then it can be abstracted away from the exercise completely, as students probably already know how to call a function and use its returned value. Validation can still take place on the native representation. On top of that, most modern frameworks that are focussing on API implementation rather than SSR HTML do already JSON parsing before handing over to the "action" or "handler".

Considering this, the exercise itself should probably be changed and loosen their requirements.

@jiegillet
Copy link
Contributor Author

jiegillet commented Jul 15, 2021

For the record, I was not planning on making students parse JSON. I did it in my example solution only so that they could pass the automated tests without using a library. Obviously that part of the code is a horrible hack that can only pass the specific tests.

What I had in mind was to let them use Jason or Poison to do the decoding. Then the tests could not run on the site, but I think that anyone that reaches that levels without having used the CLI probably ought to learn. We could potentially do a concept exercise for that? I know I've learned a lot about mix test flags because of those frustrating pending tags.

Also, the point of this exercise is not JSON parsing, even though they state that it is important to do as well. The point here is the data management.

@NobbZ
Copy link
Member

NobbZ commented Jul 15, 2021

What I had in mind was to let them use Jason or Poison to do the decoding

And this requires changes in the testrunner. Non-trivial as I understood it in slack the other day.

In general the test runner does not have networking, so a set of potential dependencies had to be held available within the runner and "injected" on demand, with versions that are within bounds.

@jiegillet
Copy link
Contributor Author

As I mentioned, we could restrict this exercise to CLI only and make that clear in the description, and also in a test message if they try to run it anyway.

@angelikatyborska
Copy link
Member

@jiegillet After a lot of thought, I have come to the conclusion that the best thing to do with this exercise is not to add it to the Elixir track. I'm really sorry that I didn't realize this earlier and didn't stop you from working on it. I was unsure...

I have to agree with @NobbZ:

At the same time, the requirement of the exercise doesn't really make sense for me.

(...)

Considering this, the exercise itself should probably be changed and loosen their requirements.

But one of the things that I am not really interested in doing as a maintainer is arguing organization-wide for and against exercise ideas, so I don't want to bring this up in problem-specs.

It seems like in its current form, the exercise would require students to use an external library, like you're saying. Both @neenjaw and I, we're not really interested in teaching people in this track about specific external libraries.

Another problem I have with this exercise is that it pretends to have something to do with web apps while having nothing to do with web apps. I could see this story being an idea for a tiny Plug/Phoenix project, but there's no room for such things on Exercism, at least not right now. But the idea of making an exception for this exercise (by having a single exercise that's not available on the web) to make people implement a GenServer to hold state that they should have written in a SQL database, to pretend that sending messages is making HTTP calls.. it just doesn't feel right to me, now that I was forced to think about it 😓.

There's a whole discussion about this in the original issue from 2018. Seems like Rust was planning to implement this by requiring an actual HTTP server (exercism/problem-specifications#1263 (comment)) but didn't implement it at all yet...

@jiegillet
Copy link
Contributor Author

jiegillet commented Jul 16, 2021

It's all good. I really respect the time and effort you and @neenjaw place into making the Elixir track coherent and high-quality. I also agree that it's not your job to manage organization-wide exercises, but rather to see how they fit or don't fit in the track. And you do that really well, this is proof of it. I'm sorry that I forced you to struggle so much about it.

On my side, I don't feel that I wasted my time, I'm doing this to learn and here I learned about :ets which wasn't even on my radar before, so it's a net positive.

If students want to learn about REST and database management and such, it's not like there are no other resources available, especially with Elixir!

I will change the PR to add the exercise to the foregone list. While I'm at it, should I add high-scores, micro-blog and reverse-string too? Then that will account for all exercises that have a canonical-data.json.

@NobbZ
Copy link
Member

NobbZ commented Jul 16, 2021

I started the discussion now, wanted to do it yesterday already, but to much of work in the office :D

@github-actions
Copy link
Contributor

Thank you for contributing to exercism/elixir 💜 🎉. This is an automated PR comment 🤖 for the maintainers of this repository that helps with the PR review process. You can safely ignore it and wait for a maintainer to review your changes.

Based on the files changed in this PR, it would be good to pay attention to the following details when reviewing the PR:

  • General steps
    • 🏆 Does this PR need to receive a label with a reputation modifier (x:size/{tiny,small,medium,large,massive})? (A medium reputation amount is awarded by default, see docs)

Automated comment created by PR Commenter 🤖.

Copy link
Member

@angelikatyborska angelikatyborska left a comment

Choose a reason for hiding this comment

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

Thank you 💙

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.

3 participants