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

manual: move quoted string to the main chapters #1788

Merged
merged 4 commits into from May 25, 2018

Conversation

Projects
None yet
4 participants
@Octachron
Copy link
Contributor

commented May 20, 2018

This PR proposes to move quoted strings outside of the "language extensions" chapter:

  • the grammar description is moved to manual/manual/lex.etex after the description of the classical string literals.

  • their use in extension nodes is now discussed in the extension nodes section. I have reworked a bit the paragraph structure to avoid few paragraph-sized sentences.

  • they briefly appear in the tutorial when strings are introduced. As a small related change, the "Hello world" introductive strings is replaced by "Hello" ^ " " ^ "world" to briefly impart the fact that the operator for string concatenation is ^.

@pmetzger
Copy link
Member

left a comment

Thank you for doing this, @Octachron. I have a few suggestions, but they are just suggestions.

represent arbitrary SQL statements -- assuming you have a ppx-rewriter
that recognizes the "%sql" extension.

Note that the non-extension form, for example "{sql|...|sql}", should

This comment has been minimized.

Copy link
@pmetzger

pmetzger May 20, 2018

Member

I would change this to "the sigil-quoted form" or "prefix-quoted form" or some such. After all, none of the {| quoted string forms are for extensions, so calling this the "non-extension form" makes it sound like the other form is only for extensions.

This comment has been minimized.

Copy link
@Octachron

Octachron May 20, 2018

Author Contributor

What do you think of "word-delimited form" ?

This comment has been minimized.

Copy link
@pmetzger

pmetzger May 20, 2018

Member

I like!

that recognizes the "%sql" extension.

Note that the non-extension form, for example "{sql|...|sql}", should
not be used for this purpose. Indeed, the user cannot see in the code that

This comment has been minimized.

Copy link
@pmetzger

pmetzger May 20, 2018

Member

I'd be even more explicit and say "...not be used for signaling an extension is in use. Indeed..."

This comment has been minimized.

Copy link
@Octachron

Octachron May 20, 2018

Author Contributor

Good idea.

The identifier @quoted-string-id@ is a (possibly empty) sequence of
lowercase letters and underscores that can be freely chosen to avoid
such issue.

This comment has been minimized.

Copy link
@pmetzger

pmetzger May 20, 2018

Member

Good, but I'd give two examples, like {|hello|} and {foo|{|hi|}there|foo}.

This comment has been minimized.

Copy link
@Octachron

Octachron May 20, 2018

Author Contributor

Probably (even if examples go a bit against the tone of the chapter).

@@ -58,7 +58,8 @@ usual basic data types: booleans, characters, and immutable character strings.
\begin{caml_example}{toplevel}
(1 < 2) = false;;
'a';;
"Hello world";;
"Hello" ^ " " ^ "world";;
{|This is a quoted string where \n is not a newline|};;

This comment has been minimized.

Copy link
@pmetzger

pmetzger May 20, 2018

Member

And perhaps a third:

{x|this {|is also|} a quoted string|x} or some such.

\begin{verbatim}
String.length {|\"|} (* returns 2 *)
String.length {foo|\"|foo} (* returns 2 *)
\end{verbatim}

This comment has been minimized.

Copy link
@pmetzger

pmetzger May 20, 2018

Member

Note that we're losing this example, perhaps move it to the tutorial? It's useful.

This comment has been minimized.

Copy link
@Octachron

Octachron May 20, 2018

Author Contributor

I would propose to add to the tutorial example

  {delimiter|the end of this  |} quoted string is here|delimiter}
= "the end of this |} quoted string is here";;

to solve both problems at the same time.

This comment has been minimized.

Copy link
@pmetzger

pmetzger May 20, 2018

Member

I like, but the dumb example String.length {|\"|} (* returns 2 *) does remain useful and might go somewhere. It explains to beginners why you would want such a thing at all, which is to avoid leaning toothpick syndrome.

This comment has been minimized.

Copy link
@Octachron

Octachron May 20, 2018

Author Contributor

I missed your point then. I would say that for this case

"\"\\\\\""={|"\\"|}

is a better example.

This comment has been minimized.

Copy link
@pmetzger

pmetzger May 20, 2018

Member

That would do it too.

@gasche

This comment has been minimized.

Copy link
Member

commented May 20, 2018

I trust @pmetzger to do a good job reviewing this PR, so I'll be happy to approve once you are both happy with the way his comments are addressed.

@Octachron Octachron force-pushed the Octachron:nme_quoted_strings branch from bd85939 to 97e70c4 May 20, 2018

not be used for this purpose. Indeed, the user cannot see in the code that
Note that the word-delimited form, for example "{sql|...|sql}", should
not be used for signaling an extension is in use.
Indeed, the user cannot see in the code that

This comment has been minimized.

Copy link
@pmetzger

pmetzger May 20, 2018

Member

Perhaps "know" rather than "see"?

This comment has been minimized.

Copy link
@Octachron

Octachron May 20, 2018

Author Contributor

I think that see is better here, since with enough context, one can know that {js|...|js} is an implicit extension node.

This comment has been minimized.

Copy link
@pmetzger

pmetzger May 20, 2018

Member

Hrm. Maybe then I misunderstand the point being made?

This comment has been minimized.

Copy link
@Octachron

Octachron May 20, 2018

Author Contributor

Or I was unclear. What I mean is that with [%sql {|..|}], it is possible to see directly that this is an extension. With {js|...|js} it could be either a legitimate use of quoted strings, or some ppx stealing the quoted string syntax. I cannot see, locally, what is the expected semantics and I need to inquire about the current context; but I can still (hopefully) know. So I think that the immediacy of see is a good point here.

This comment has been minimized.

Copy link
@pmetzger

pmetzger May 20, 2018

Member

Okay, now I get it. Some suggested edits to reinforce that:

Note that the word-delimited form, for example "{sql|...|sql}", should not be used for signaling that an extension is in use.
Indeed, the user cannot see from the code whether this string literal has a different semantics than they expect.
Moreover, giving a semantics to a specific delimiter limits the freedom to change the delimiter to avoid escaping issues.

[Note the struck out "a"s in front of "semantics" and "different semantics", it's hard to see the line through them. I did this because "semantics" is a plural mass noun and thus doesn't take the "a" article except, sadly, when it does take it as in "I've created a formal semantics for C" — English is a horrid and illogical language. The wording also reads a bit better with the other slight adjustments indicated in bold italic.]

This comment has been minimized.

Copy link
@Octachron

Octachron May 20, 2018

Author Contributor

This sounds better indeed, thanks!

@pmetzger

This comment has been minimized.

Copy link
Member

commented May 20, 2018

I'm pretty happy at this point.

@gasche

gasche approved these changes May 20, 2018

@@ -54,11 +54,23 @@ fib 10;;
\pdfsection{Data types}

In addition to integers and floating-point numbers, OCaml offers the
usual basic data types: booleans, characters, and immutable character strings.
usual basic data types: booleans,

This comment has been minimized.

Copy link
@xclerc

xclerc May 21, 2018

Contributor

It looks like the enumeration (and the sentence as a consequence)
is not properly terminated.

This comment has been minimized.

Copy link
@pmetzger

pmetzger May 21, 2018

Member

@xclerc Explain? I'm not sure I get your point.

This comment has been minimized.

Copy link
@xclerc

xclerc May 21, 2018

Contributor

Well, if I am correct, the paragraph is "In addition to integers and floating-point
numbers, OCaml offers the usual basic data types: booleans,
", then followed by
an example. It seems incorrect to end the paragraph with a comma (and having
an enumeration with only one item is unexpected).

This comment has been minimized.

Copy link
@pmetzger

pmetzger May 21, 2018

Member

It isn't the end. There are two other things in the enumeration, "characters" and "strings". What's happened is that examples have been interspersed after each data type.

This comment has been minimized.

Copy link
@xclerc

xclerc May 21, 2018

Contributor

Oh, I see -- sorry for the noise.

This comment has been minimized.

Copy link
@Octachron

Octachron May 21, 2018

Author Contributor

Nevertheless, code examples interleaved with the text (and integrated with the punctuation) might be unusual. Should I switch to a (bullet point) list to improve readability?

This comment has been minimized.

Copy link
@pmetzger

pmetzger May 21, 2018

Member

I haven't actually formatted the manual with your changes (I read the LaTex directly) so I'm not sure. Why don't you look at it both ways and pick one? I'm sure you'll pick reasonably.

This comment has been minimized.

Copy link
@Octachron

Octachron May 23, 2018

Author Contributor

After taking a look at both options, I picked the itemized list, since I found that the indentation gives better visual cues to the reader.

This comment has been minimized.

Copy link
@pmetzger

pmetzger May 25, 2018

Member

Seems good to me. Is this a candidate for cherry-picking to 4.07?

@Octachron Octachron merged commit 1d27dde into ocaml:trunk May 25, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.