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

revert #17315 (keeping good parts) #17528

Closed
wants to merge 1 commit into from
Closed

Conversation

a-mr
Copy link
Contributor

@a-mr a-mr commented Mar 26, 2021

@timotheecour , @xflywind., I have to take back my approve of #17315 . I'm very sorry because I also added some confusion.

New examples presented by @timotheecour in nim-lang/RFCs#355 (comment) showed that #17315 was a really bad idea.

We lost the way to format code like:

See this Bash code: `export VAR=\`pwd\`; \\`.

It worked fine before:

image

The essence of the entire problem is that rst2html.py has 2 backslash modes:

  1. escape backslashes, which was used between backticks before
  2. not escape which fix #17260 render \ properly in nim doc, rst2html #17315 moved to. (This also matches the actual behavior of rst2html.py with :code: role)

We really should stick to number 1 because:

  • it's the only mode that is described in the RST spec. It's obvious that @Araq wrote rst.nim code with accordance with the spec.
  • it's simple and easy to remember. Even if it requires to type double \\ it's OK — it matches Nim programming languages escaping in strings "\\"
  • it applies to any other part of RST in general
  • it's universal: see the example above. (it even allows trailing whitespaces via syntax `beginning of code \ `, though that currently did not work in Nim)

The good things of 2. is that it matches Markdown behavior better and that it allows to type backslashe as single backslash (unescaped).

Now I actually think that the behavior of rst2html.py with :code: role should never be replicated in Nim. It's a wrong design. Let us just proclaim that we conform to the spec better.

In this PR I left relevant test cases from #17315 and changed the function doc comment from original bug #17260 to render properly with old syntax.

cc @narimiran

@a-mr
Copy link
Contributor Author

a-mr commented Mar 26, 2021

Also I thought that it's possible to escape backtick rst2html.py \`. And @timotheecour implemented it that way in #17315.

But I was wrong: it's not escaped, it just repeats \ and ` in rst2html.py. I don't know what the author of rst2html.py thought when he did it this way.

It means that we are not going to match rst2html.py with #17315 anyway.

(If sticking with original @timotheecour idea that allowed \` then would lose ability to type Nim symbols containing ` between backticks).

@a-mr a-mr changed the title revert #17315 (leaving good parts) revert #17315 (keeping good parts) Mar 26, 2021
@a-mr
Copy link
Contributor Author

a-mr commented Mar 26, 2021

So the only downside with the initial syntax is that backslashes \ should be input either as

` ... \\ ... `

or as

`` ... \ ...``

Let us leave it so. It's a small price for consistency.

@a-mr
Copy link
Contributor Author

a-mr commented Mar 26, 2021

Though may be I'm attaching too much significance to this stuff.

May be we should just document the current behavior and that would be enough.

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.

1 participant