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

update nimgrep documentation #17415

Merged
merged 2 commits into from
Mar 23, 2021
Merged

update nimgrep documentation #17415

merged 2 commits into from
Mar 23, 2021

Conversation

a-mr
Copy link
Contributor

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

The options are extracted into a separate files and a few RST formatting issues were fixed accordingly. A few examples were also added.

@a-mr a-mr force-pushed the nimgrep-doc-upd branch from eef9ccb to 8517b0d Compare March 18, 2021 21:02
@@ -2,17 +2,20 @@
nimgrep User's manual
=========================

.. default-role:: literal
Copy link
Member

Choose a reason for hiding this comment

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

why not code? it renders differently for things like backslash IIRC; code is the closest to verbatim rendering (and to markdown)

.. default-role:: code is what I've used in #17028 (and manual since #17259)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because code is intended for code in programming languages. It expects highlighting. This is an actually implemented feature in rst2html.py:

.. role:: nim(code)
   :language: nim

.. default-role:: nim

Some code: `when PrintRopeCacheStats: echo "rope cache stats: "`.

Another example: `from ic / ic import rodViewer`.

image

It uses pygmentize to highlight code. And pygmentize knows Nim!

I think one day we will implement that too.

...And here in nimgrep.rst all quotes are used either for option names or some commands, which are not exactly code (may be viewed as Bash code though or any other shell without specifics). All those cases match literal role perfectly.

Copy link
Member

Choose a reason for hiding this comment

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

ok! maybe keep this comment open as it has useful information (or link to it from #17340)

:Author: Andreas Rumpf
:Version: 0.9
:Version: 1.6.0
Copy link
Member

@timotheecour timotheecour Mar 19, 2021

Choose a reason for hiding this comment

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

that version is specific to nimgrep; maybe bump it to 0.10.0 instead

Copy link

Choose a reason for hiding this comment

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

@timotheecour nimgrep is 1.6.0 though

Copy link

Choose a reason for hiding this comment

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

Copy link
Member

@timotheecour timotheecour Mar 19, 2021

Choose a reason for hiding this comment

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

ok (a bit confusing since matches next nim version but i guess that's a coincidence)
btw @a-mr is there a way to insert comments to rst such that they won't render in rendered html? that would be useful for situations like this (and many others), eg:

.. hidden_comment::
  the version is independent from nim version

EDIT: found it:

before

..
  some comment that will not show

after

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, equal versions is a pure coincidence.

--recursive --ext:'nim|nims' --ignoreStyle
# short: -r --ext:'nim|nims' -y

.. Note:: we used `'` quotes to avoid special treatment of `|` symbol
Copy link
Member

@timotheecour timotheecour Mar 19, 2021

Choose a reason for hiding this comment

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

pre-existing but IMO we should allow , and undocument |:

  • anything that requires escaping just adds complexity
  • , is more standard in cmdline utilies, | is almost never used for such purpose for this very reason (inside regex is fine though)

+ Nimgrep can search multi-line, e.g. to find files containing `import`
and then `strutils` use::

'import(.|\n)*?strutils'
Copy link
Member

Choose a reason for hiding this comment

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

  • can you make this more precise via \b:
Suggested change
'import(.|\n)*?strutils'
'import(.|\n)*?\bstrutils\b'
  • doesn't that have false positives, eg:
import foo
# see also strutils

?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  • I want to keep this example simple but still usable
  • yes, it will match even if this "strutils" comment is situated somewhere in the end of file. But Regex is not intended for good semantic checking, right? I mean using it from cmdline supposes quick-and-dirty patterns and hence a number of false positives. (I've heard that it's possible to parse pretty complex grammars by Regex in theory -- I'm not a CS guy though)

Copy link
Member

Choose a reason for hiding this comment

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

ok, but at least mention this in the comment, eg:

+ Nimgrep can search multi-line, e.g. to find files containing `import`
  and then `strutils` use this (this can have false positives)::


.. Attention:: note the subtle difference between `--excludeDir` and
`--includeDir`: the former is applied to relative directory entries
and the latter is applied to the whole paths
Copy link
Member

Choose a reason for hiding this comment

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

pre-existing but what's the rationale for that?
IMO ripgrep has a saner inclusion/exclusion syntax

Copy link
Contributor Author

Choose a reason for hiding this comment

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

speed and simplicity of implementation.

--excludeDir is easy to implement as sub-directory name exclusion: just don't step into any matching sub-directory.

For --includeDir one needs to make sure that any of sub-directories in the path does not match to throw it away.

E.g. if we do --includeDir:subdir2 here

subdir1/subdir2/subdir3/file

we can't stop when subdir1 were not matched or subdir3 not matched.


Usage:

* To search::
Copy link
Member

@timotheecour timotheecour Mar 19, 2021

Choose a reason for hiding this comment

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

I understand you use :: to make rst2html happy, but this now makes nimgrep -h show :: and other artifacts introduced in this PR (eg .. Note::)

I completely agree with de-duplicating the help message, but we should address this.

rST's include directive provides a parameter code for including files that should be parsed as source code which may be highlighted. This is missing in Nim

Copy link
Contributor

Choose a reason for hiding this comment

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

I have a pr I've been working on for nimgrep. I deleted the old rst file and wrote this to keep it dry::

proc staticParseVerAndUsageFromDoc: (string, string) {.compileTime.} =
  ## Reads in this source file & parses out the title, version, author,
  ## copyright and cli help info from module's doc.
  ## There are many options to `nimgrep` and the old docs diverged over
  ## the years.
  template slice(offset: int, mark: string, endMark = "\n"): untyped =
    let off0 = source.find(mark, offset) + mark.len
    doAssert off0 > -1 + mark.len, "(" & mark.escape & " .. " & endMark.escape & ")"
    let off1 = source.find(endMark, off0) - 1
    doAssert off1 > -1, "(" & mark.escape & " .. " & endMark.escape & ")"
    off0..off1
  let source = staticRead currentSourcePath
  const doc0Mark = "\n\n## ="
  let doc0 = source.find(doc0Mark)
  doAssert doc0 > -1
  let title = slice(doc0 + doc0Mark.len, "=\n## ")
  let author = slice(title.b, "## :Author: ")
  let copyright = slice(author.b, "## :Copyright: ")
  let version = slice(copyright.b, "## :Version: ")
  var usage = slice(version.b, "\n## Usage\n## =", "\n## Design Notes\n## =")
  usage.a -= "\n## Usage\n## =".len
  result[0] = source[version]
  result[1] = "\n" & source[title] &
              "\n\n  Version " & source[version] &
              "\n  (c) " & source[copyright] & " " & source[author] &
              "\n" & source[usage]
  result[1] = result[1].multiReplace([("\n## ", "\n  "), ("\n##", "\n"),
                                      ("::\n", ":\n")])

const (Version, Usage) = staticParseVerAndUsageFromDoc()

Copy link
Contributor

Choose a reason for hiding this comment

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

That of course was after I reformatted and added more docs such as a "Design Notes" section that was used the boundry of the cli usage sections.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@quantimnot do you mean you put the whole nimgrep.rst inside nimgrep.nim as a doc comment?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes. It was really outdated compared to the source. So I deleted it and added lots of docs to the source.

I was exploring other changes to nimgrep. my nimgrep wip branch

Copy link
Contributor Author

@a-mr a-mr Mar 19, 2021

Choose a reason for hiding this comment

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

This approach makes sense to avoid documentation getting out of sync but I'm not sure that doc comments are appropriate for writing User's Manual.

The 2 problems with it:

  1. file nimgrep.nim gets larger, though we should probably move some code to separate file(s) anyway
  2. we may want to have internal (developer) documentation that can be generated by nim doc --docInternal. And this approach mixes Manual and Developer docs together.

May be it makes sense to introduce a next level of doc comments ### specifically for writing User's Manuals, Guides, etc.

@timotheecour wdyt?

Copy link
Member

Choose a reason for hiding this comment

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

There is nothing wrong with having nimgrep's documnetation in the nimgrep.rst file. Having this in the source code instead is not acceptable. Yes, yes, maybe it makes it slowly harder to change nimgrep's command line interface as the documentation then also needs to be updated. But that's a good thing -- some users trust nimgrep's command line interface not to change all the time...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Araq

So what do you think about this PR? Say clearly please:

  • yes, it's good to put cmdline options into nimgrep_cmdline.txt for both nimgrep.nim and nimgrep.rst
  • no, it's better to have the options described separately even at the cost of duplication

Copy link
Member

@timotheecour timotheecour Mar 20, 2021

Choose a reason for hiding this comment

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

I can't speak for araq but as far as I'm concerned, the way you did it by introducing nimgrep_cmdline.txt (included by both nimgrep.nim and nimgrep.rst) is indeeed the way to go (and in fact we do the exact same with basicopt.txt); there's no need for duplication, which is always bad. And yes, it also allows having the documentation in a separate nimgrep.rst + nimgrep_cmdline.txt file.

The only thing is it needs a (hopefully simple) post-processing so that it renders well in both cmdline output and rst docs, but that can be done in a followup PR if needed. The post-processing can be reused in all similar instances and doesn't have to be specific to nimgrep, in fact we can reuse same post-processing for basicopt.txt so that nimc.rst looks better.

Copy link
Member

Choose a reason for hiding this comment

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

I agree with @timotheecour

doc/nimgrep_cmdline.txt Outdated Show resolved Hide resolved
@Araq
Copy link
Member

Araq commented Mar 23, 2021

Merging now but later we should make the help text less excessive. It should be a page of options and then a link to the full documentation.

@Araq Araq merged commit 368422c into nim-lang:devel Mar 23, 2021
@timotheecour timotheecour added the TODO: followup needed remove tag once fixed or tracked elsewhere label Mar 24, 2021
ardek66 pushed a commit to ardek66/Nim that referenced this pull request Mar 26, 2021
* update nimgrep documentation

* Update doc/nimgrep_cmdline.txt

Co-authored-by: Andreas Rumpf <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
TODO: followup needed remove tag once fixed or tracked elsewhere
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants