Skip to content

Conversation

@c-cube
Copy link
Contributor

@c-cube c-cube commented May 22, 2023

No description provided.

@c-cube c-cube requested a review from christinerose as a code owner May 22, 2023 15:11
@cuihtlauac
Copy link
Collaborator

Thanks @c-cube, am I correct assuming this only about the Sequences documentation and the other files went in this PR accidentally?

@sabine
Copy link
Collaborator

sabine commented May 22, 2023

This might be the state of staging at the time.

@c-cube
Copy link
Contributor Author

c-cube commented May 22, 2023

Ok I have no idea what's going on there. I edited from the "contribute" button on the website (URL: https://github.com/ocaml/ocaml.org/blob/c461890f0ce6473bd4faf55b1d7157c1000e0fcb/data/tutorials/ds_05_seq.md) and ended up on a detached commit?

@sabine
Copy link
Collaborator

sabine commented May 22, 2023

Sorry about that, the contribute link goes to the commit on the staging branch.

@c-cube
Copy link
Contributor Author

c-cube commented May 22, 2023

if someone wants to cherry pick the small change… :)

@sabine
Copy link
Collaborator

sabine commented May 22, 2023

So the diff with staging instead of main is going to be much better. 🙂

No worries, we will take it from here.

@c-cube
Copy link
Contributor Author

c-cube commented May 22, 2023

Thanks y'all :)

@tmattio tmattio changed the base branch from main to staging May 22, 2023 16:08
@cuihtlauac
Copy link
Collaborator

I've updated #791 with what seemed unique to this PR. Changes had taken place in parallel there. Reconciliation was needed. I hope I haven't lost any of your suggestions in the process.

Thanks again for your feedback, and apologizes for the unpleasant review experience.

staging is up to date w.r.t. #791

@c-cube
Copy link
Contributor Author

c-cube commented May 22, 2023

I don't think you lost anything! If anything you attributed too many things to me in the last commit 😅 . I'm closing this.

No worries for the reviewing experience, it's never easy to make this kind of things smooth. Cheers!

@c-cube c-cube closed this May 22, 2023
Copy link
Contributor

@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.

Mostly minor grammar/formatting fixes. Tightened some syntax and found a few typos.

function returning a sequence while its `list` counterpart returns a list. Let's
compare the constructors of `list` and `Seq.node`:
1. Empty lists and sequences are defined the same way, a constructor without any
parameter: `Seq.Nil` and `[]`.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
parameter: `Seq.Nil` and `[]`.
parameters: `Seq.Nil` and `[]`.

```ocaml
# Seq.iter print_int (ints 0);;
```
in an OCaml top-level means “print integers forever,” and you have to press
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
in an OCaml top-level means “print integers forever,” and you have to press
in an OCaml toplevel means “print integers forever,” and you have to press

The function `sieve` is recursive in OCaml and common sense. It is defined using
the `rec` keyword and calls itself. However, some call that kind of function
[_corecursive_](https://en.wikipedia.org/wiki/Corecursion). This word is used to
emphasize that, by design, it does not terminate. Strictly speaking, the sieve
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
emphasize that, by design, it does not terminate. Strictly speaking, the sieve
emphasise that, by design, it does not terminate. Strictly speaking, the sieve

* Sequence consumer: partially applied function parameter
* Sequence producer: partially applied function result

When code dealing with sequences does not behave as expected like if it is
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
When code dealing with sequences does not behave as expected like if it is
When code dealing with sequences does not behave as expected, like if it is

val String.of_seq : string -> char Seq.t
val String.to_seq : char Seq.t -> string
```
Similar functions are also provided for sets, maps, hash tables (`Hashtbl`) and
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Similar functions are also provided for sets, maps, hash tables (`Hashtbl`) and
Similar functions are also provided for sets, maps, hash tables, (`Hashtbl`) and


<p>
The <a href="<%s Url.tutorial "up-and-running" %>#installing-the-ocaml-platform-tools">OCaml Platform Tools</a>,
which include the <a target="_blank" href="https://dune.build/">build system <tt>dune</tt></a>,
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
which include the <a target="_blank" href="https://dune.build/">build system <tt>dune</tt></a>,
which include the <a target="_blank" href="https://dune.build/">build system Dune</a>,

<p>
The <a href="<%s Url.tutorial "up-and-running" %>#installing-the-ocaml-platform-tools">OCaml Platform Tools</a>,
which include the <a target="_blank" href="https://dune.build/">build system <tt>dune</tt></a>,
complete your OCaml development environment. To install them in your current <code>opam</code> switch, run this command:
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
complete your OCaml development environment. To install them in your current <code>opam</code> switch, run this command:
complete your OCaml development environment. To install them in your current opam switch, run this command:

<li>
<h3>Use the Diskuv OCaml ("DKML") Windows installer</h3>
<p>
The installer sets up OCaml 4.14.0 and <a target="_blank" href="https://github.com/ocaml/opam">OCaml's package manager <tt>opam</tt></a>,
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
The installer sets up OCaml 4.14.0 and <a target="_blank" href="https://github.com/ocaml/opam">OCaml's package manager <tt>opam</tt></a>,
The installer sets up OCaml 4.14.0 and <a target="_blank" href="https://github.com/ocaml/opam">OCaml's package manager opam</a>,

</a>

<p>
Before you run the installer: Make sure your Windows username does <b>not</b> contain a space character (e.g. for <code>C:\Users\Jane Smith</code>, OCaml will not install properly).
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Before you run the installer: Make sure your Windows username does <b>not</b> contain a space character (e.g. for <code>C:\Users\Jane Smith</code>, OCaml will not install properly).
Before you run the installer: Make sure your Windows username does <b>not</b> contain a space character (e.g., for <code>C:\Users\Jane Smith</code>, OCaml will not install properly).

<h2>You have an OCaml Development Environment</h2>

<p>
The <a href="<%s Url.tutorial "up-and-running" %>#installing-the-ocaml-platform-tools">OCaml Platform Tools</a> include the <a target="_blank" href="https://dune.build/">build system <tt>dune</tt></a>,
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
The <a href="<%s Url.tutorial "up-and-running" %>#installing-the-ocaml-platform-tools">OCaml Platform Tools</a> include the <a target="_blank" href="https://dune.build/">build system <tt>dune</tt></a>,
The <a href="<%s Url.tutorial "up-and-running" %>#installing-the-ocaml-platform-tools">OCaml Platform Tools</a> include the <a target="_blank" href="https://dune.build/">build system Dune</a>,

Copy link
Contributor

@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.

Mostly minor grammar/formatting fixes. Tightened some syntax and found a few typos.

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.

4 participants