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

Add rst parse option for continuing after parse error #14280

Closed
wants to merge 4 commits into from

Conversation

jyapayne
Copy link
Contributor

@jyapayne jyapayne commented May 9, 2020

Fixes: #8615

Adds the ability to specify a compiler flag to ignore rst parsing errors. This comes in handy when generating documentation or when the nim forum needs to render rst to html.

@jyapayne jyapayne force-pushed the add-rst-parse-option branch from aed8fb5 to edcad00 Compare May 9, 2020 02:12
@jyapayne jyapayne changed the title Add rst parse option Add rst parse option for continuing after parse error May 9, 2020
arg: string) {.procvar.} =
let mc = msgkind.whichMsgClass
let a = messages[msgkind] % arg
let message = "$1($2, $3) $4: $5" % [filename, $line, $col, $mc, a]
Copy link
Member

Choose a reason for hiding this comment

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

import std/miscdollars
and use toLocation which I've added recently as the single place where location formatting is defined (to allow easy override)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Alright. Should I change the proc above as well?

Copy link
Member

Choose a reason for hiding this comment

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

somehow I missed that; yes por favor! (or in a different "cleanup" PR depending on whether this is staling or not)

@timotheecour
Copy link
Member

timotheecour commented May 9, 2020

I'm not yet convinced; can something like #14282 work instead? I can move raiseParseError to a reusable location; and add an optional file/line/col argument ; (this is meant to be usable in other contexts eg json parse errors)

do you have a minimal example where something like #14282 would not be sufficient?

This comes in handy when generating documentation

IMO that should just error, but give a nice error with the sufficient context (possible with smthg like #14282 )

or when the nim forum needs to render rst to html.

likewise

@jyapayne
Copy link
Contributor Author

jyapayne commented May 9, 2020

@timotheecour In nimterop, @genotrance and I would like the ability to include arbitrary C docs and have them render regardless of if they contain errors or not. #14282 is nice, but it would not allow for this feature since it would just error and not include the docs at all, which is annoying at best and sometimes a huge pain.

@timotheecour
Copy link
Member

timotheecour commented May 9, 2020

so I guess it relates to #13638 and nimterop/nimterop#61.
Still not convinced this is the correct approach. How about this instead:

# add this:
--docfmt:fmt # fmt in rst,md,doxygen,raw,besteffort (for now just rst,raw)
# if not set, `--docfmt:rst` is implied (ie, set in newConfigRef())

right now only rst is supported but your use case would add raw (un-interpreted), which is easy to support and add.

That way, it allows you to check your assumptions about comment format, and if comments are in a different format to begin with, unless we have an easy way to convert to rst, there's no point in trying to format to rst in the 1st place (eg it could give wrong results yet make the parser pass without error)

also if you have concrete example file showing those C doc comments that would help this discussion

@jyapayne
Copy link
Contributor Author

jyapayne commented May 9, 2020

@timotheecour that would be ideal. I would love to just remove the rst formatting in the first place. I agree it works better and would be fine with that solution.

@timotheecour
Copy link
Member

timotheecour commented May 9, 2020

  • so you could start with just --docfmt:rst|raw (should be fairly simple patch) and it'd solve at least this problem for now, with the advantage that it's extensible to other formats (eg, should someone start supporting md ; already have libs for that)

raw is un-interpreted so cannot raise

note:

instead of a new cmdline flag, one option is a pushable pragma: so that nimterop can simply have this:

{.push docfmt:"raw".}
# .. nimterop generated code
{.pop.}

and users don't need to have a custom command line invocation to generate docs

@jyapayne
Copy link
Contributor Author

jyapayne commented May 9, 2020

instead of a new cmdline flag, one option is a pushable pragma: so that nimterop can simply have this

I like that idea too, but have no idea how to implement it. I think for now it's simpler to do the --docfmt change.

@timotheecour
Copy link
Member

timotheecour commented May 9, 2020

the more I think of it the more the pragma makes sense. eg:
nim doc --project main would be incorrect with a global --docfmt if your project has modules that have classical doc comments (rst) and modules that have raw comments (eg nimterop wrapper).

This really is a module (or scope)-level annotation, not something a user should configure.

I like that idea too, but have no idea how to implement it.

i can help if needed. As usual, easiest is to pick something that's similar in the compiler and track the code to see how it's implemented.

You need to propagate from a {.docfmt:"raw".} to a some flag that is accessible in genComment(d: PDoc, n: PNode): string.

One option is to restrict this feature to be at module scope for now, and then future implementations can relax that. So you could add a member in TSymFlag and attach it to the module where the pragma is defined.

Beyond the wrapper use case for foreign/C doc comments, this feature could in future be used to allow writing your doc comments in markdown in a given module (via {.docfmt:md.}) without breaking anything, which I'm sure many would appreciate.

In any case, check with @Araq to make sure this can be accepted.

@Araq
Copy link
Member

Araq commented May 11, 2020

I don't like it, usually the documentation is already bad enough, adding an option for having even less quality is a bad idea. What does "Ignore parsing errors" even mean? That effectively adds an unspecified error recovery mechanism to the existing RST parser.

@jyapayne
Copy link
Contributor Author

@Araq what about the option for --docfmt=raw?

@Araq
Copy link
Member

Araq commented May 11, 2020

Instead our RST/Markdown parser (yes, for the 100th time, it also does support markdown!) should get support for special syntactic construct where plaintext is inserted. Something like:

.. code-block:: plaintext

Or the same with triple backticks which I cannot write down here because markdown's triple backticks do not nest.

@kaushalmodi
Copy link
Contributor

kaushalmodi commented May 12, 2020

because markdown's triple backticks do not nest.

They do.. increase the number of backticks in the nester block. e.g.

This is wrapped in 5 back ticks
|   ````
|   This is wrapped in 4 back ticks
|   |   ```
|   |   This is wrapped in 3 back ticks
|   |   ```
|   ````

@timotheecour
Copy link
Member

timotheecour commented May 12, 2020

They do.. increase the number of backticks

TIL...

proc Mat*(rows: cint, cols: cint, `type`: cint) {.importcpp:"Mat::Mat(@)".} =
  ##[```
  @overload
  @param rows Number of rows in a 2D array.
  @param cols Number of columns in a 2D array.
  @param type Array type. Use CV_8UC1, ..., CV_64FC4 to create 1-4 channel matrices, or
  CV_8UC(n), ..., CV_64FC(n) to create multi-channel (up to CV_CN_MAX channels) matrices.
  ```]##
  runnableExamples: discard

image

not too bad?
so maybe what's needed is a way to convert a raw comment (eg from nimtero/treesitter) into something properly escaped so that it won't trip off rst (in case of odd nested triple backticks) but seems rare

@jyapayne WDYT ?

EDIT: it may also need to parse and remove the C++ comment markers (/** + * on each line), eg:

/**
 * A brief history of JavaDoc-style (C-style) comments.
 *
 * This is the typical JavaDoc-style C-style comment. It starts with two
 * asterisks.
 *
 * @param theory Even if there is only one possible unified theory. it is just a
 *               set of rules and equations.
 */

not sure whether this should be handled in stdlib, docgen, or some nimble package though

@jyapayne
Copy link
Contributor Author

Hmm, I'll have to test it out!

@jyapayne
Copy link
Contributor Author

@timotheecour The triple backticks seems to work for now. Closing this

@jyapayne jyapayne closed this May 14, 2020
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.

rstToHtml: Option to not throw error when parsing
4 participants