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

DOC: replace dream with sexp in first program #2093

Merged
merged 8 commits into from
Apr 4, 2024

Conversation

heathhenley
Copy link
Contributor

Related to #1962

What does everyone think about something like this? I tested on windows and it works, the example is just pulled from Yojson's readme, but we could make it 'learning ocaml' specific if you prefer.

Two things:

  • after installing the package using opam, I had to run the opam env update described here: https://ocaml.org/docs/installing-ocaml#2-create-an-opam-switch (not creating the switch just update the env). I'm wondering if that should be mentioned in the tutorial? Opam does tell you to run eval $(opam env) but that only helps if you're running linux based...
  • is there a better way to describe the type that Yojson returns that is more instructive for beginners?

@F-Loyer
Copy link
Contributor

F-Loyer commented Feb 25, 2024

I guess I was too fast when suggesting yojson. It is small, not system-dependant, useful... but it uses polymorphic variants which I don't think they are introduced in the learning/language section.

sexplib belongs to the same kind of library, but Sexp instead of JSON. The result type doesn't use polymorphic variants, but simple ADT.

@heathhenley
Copy link
Contributor Author

Ah, no worries I'll give that one a shot instead. I went with JSON because it was more familiar, now I get to learn what S expression is 🤣

@F-Loyer
Copy link
Contributor

F-Loyer commented Feb 25, 2024

"is there a better way to describe the type that Yojson returns that is more instructive "

Some programs need a configuration file. Then if we read it, it can be convenient to parse it, especially if the config is complex (variable assigned to list...). Then a program which extract a value could demonstrate an interest.

JSON and Sexp both permit this kind of configuration.

Note Polymorphic variants ADT are very close to the usual ADT... Perhaps we could use them.

(You already have typed a Sexp... dune file are like this).

@cuihtlauac cuihtlauac changed the title DOC: switch dream and yojson in first program DOC: replace dream with yojson in first program Mar 4, 2024
@heathhenley
Copy link
Contributor Author

Here's another option based on the suggestion of Sexplib - let me know what you all think

```
The JSON string is parsed into a [JSON type](https://ocaml-community.github.io/yojson/yojson/Yojson/Safe/index.html) so that you can use it in your program. Refer to the [Yojson documentation](https://ocaml-community.github.io/yojson/yojson/index.html) for more information.
The string you entered representing a valid S-expression is parsed into `List`of `List`s of `strings` so that you can use it in your program. Refer to the [Sexplib documentation](https://github.com/janestreet/sexplib) for more information.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
The string you entered representing a valid S-expression is parsed into `List`of `List`s of `strings` so that you can use it in your program. Refer to the [Sexplib documentation](https://github.com/janestreet/sexplib) for more information.
The string you entered representing a valid S-expression is parsed into `List` of `List`s of `strings` so that you can use it in your program. Refer to the [Sexplib documentation](https://github.com/janestreet/sexplib) for more information.

Copy link
Contributor Author

@heathhenley heathhenley Mar 11, 2024

Choose a reason for hiding this comment

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

Thanks for the corrections! A pushed a change to the type definition thanks to the clarification by @F-Loyer here so this suggestion looks outdated. Though based on their correction (that I just noticed) this probably needs another tweak to be more correct...

Copy link
Contributor

@F-Loyer F-Loyer Mar 11, 2024

Choose a reason for hiding this comment

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

Beware, I was wrong in my first comment (I have edited). It is not a list of atom (string) or sexp, but an atom (string) or a list of s-exp.

Copy link
Collaborator

@christinerose christinerose left a comment

Choose a reason for hiding this comment

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

A few suggestions and typos.

@christinerose
Copy link
Collaborator

I see several of my suggestions are now outdated, so I'll go through it again when the content is set. 🙂

@heathhenley
Copy link
Contributor Author

I updated the type def - I think it's ready for a re-review / input whether it's a good enough example to switch for dream.

Copy link
Collaborator

@christinerose christinerose 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!

@sabine sabine requested a review from cuihtlauac March 29, 2024 14:02
@cuihtlauac cuihtlauac changed the title DOC: replace dream with yojson in first program DOC: replace dream with sexp in first program Apr 2, 2024
Copy link
Collaborator

@cuihtlauac cuihtlauac left a comment

Choose a reason for hiding this comment

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

This looks good to me, only small changes suggested.

Co-authored-by: Cuihtlauac Alvarado <[email protected]>
@cuihtlauac
Copy link
Collaborator

@jonahbeckford: What's your view on this PR? It should be good for Windows?

@jonahbeckford
Copy link
Contributor

It works on Windows, yes. (I can't think of a more portable example, although I'm not enthused about the example itself. Better than the alternate so LGTM)

@cuihtlauac cuihtlauac merged commit e3e32fc into ocaml:main Apr 4, 2024
3 checks passed
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.

5 participants