add a parent directive to @section #97

martindemello · 2017-01-26T00:33:05Z

Fixes #96 by adding an optional @parentsection directive to be used with the @section directive.

dbarnett · 2017-01-26T01:19:12Z

vimdoc/docline.py

    if ident is None:
      # If omitted, it's the name in lowercase, with spaces converted to dashes.
      ident = self.name.lower().replace(' ', '-')
    self.id = ident

+    # This is potentially a problem if the parent layer name used spaces and an
+    # id was autogenerated; the parent_id needs to match the hyphenated form.
+    # We could enforce an explicit id field for sections that are used as a


Enforcing an explicit ID sounds reasonable to me.

realised this was not possible because we don't know if the parent has been read at the time we read the child, and when we assemble the tree we don't know if the id was explicit or autogenerated. i added a hint in the error message instead.

dbarnett · 2017-01-26T01:44:17Z

vimdoc/module.py

+    to_delete = []
+    for key in self.sections:
+      section = self.sections[key]
+      if 'parent_id' in section.locals and section.locals['parent_id']:


I'd avoid the try / except KeyError over the whole block when it's just one of several lookups you're trying to catch (a typo in 'children' or something could result a misleading NoSuchSection(parent_id). Also you can use setdefault and other python idioms to tighten up the code so it's easier to understand at a glance (to my eyes, anyway):

parent_id = section.locals.get('parent_id', None) if parent_id: if parent_id not in self.sections: raise error.NoSuchSection(parent_id) parent = self.sections[parent_id] parent.locals.setdefault('children', []).append(section) to_delete.append(key)

(That's one suggested alternative. Feel free to pick and chooses parts of that to suit your taste.)

thanks, didn't know about setdefault. done.

dbarnett · 2017-01-26T01:45:05Z

vimdoc/module.py

+      if 'children' in section.locals:
+        sort_key = lambda s: s.locals['name']
+        for child in sorted(section.locals['children'], key = sort_key):
+          child.locals['level'] = section.locals['level'] + 1


Any reason not to use += 1 here?

they're two different objects :)

Oh yeah, so they are…

dbarnett · 2017-01-26T01:48:10Z

vimdoc/module.py

+        section.locals['level'] = 0
+      if 'children' in section.locals:
+        sort_key = lambda s: s.locals['name']
+        for child in sorted(section.locals['children'], key = sort_key):


Nit: key=sort_key w/o spaces per https://google.github.io/styleguide/pyguide.html?showone=Whitespace#Whitespace:

Don't use spaces around the '=' sign when used to indicate a keyword argument or a default parameter value.

dbarnett · 2017-01-26T01:52:20Z

vimdoc/regex.py

  )
  # Optional identifier
  (?:
    # Separated by comma and whitespace.
-    ,\s*
+    \s*,\s*


Seems unnecessary to me accepting extra whitespace before the comma here. Is that intentional?

yes, there was an issue where the regexp was slurping spaces after the name in the case "name < parent", so i enforced the name ending with a nonspace and then i figured we might as well allow whitespaces around both the , and < separators for consistency.

Well FWIW, I would expect it allowed around the < but expect it not allowed around , (just the way commas work typographically).

okay, makes sense. taking that bit back out

dbarnett

Thanks for implementing this! (And for bearing with all my comments.)

dbarnett · 2017-01-27T05:06:49Z

README.md

@@ -114,12 +114,15 @@ Available block directives include:
 - `@public` marks a function public. In most plugins, functions are private by
  default, though this default may be overridden on a per-plugin basis.
 - `@private` marks a function private.
- `@section name[, id]` allows you to write a new section for the helpfile. The
-  id will be a lowercased version of name if omitted.
+- `@section name[, id]` allows you to write a new section for the


Nit: Looks like there's a spurious edit wrapping this line early even though it's otherwise identical.

dbarnett · 2017-01-27T05:09:04Z

README.md

+- `@section name[, id]` allows you to write a new section for the
+  helpfile. The id will be a lowercased version of name if omitted.
+- `@parentsection id` defines the current section as a child of the given
+  section. Must be contained within a @section block.


Add backticks around this @section for code font, and the other @parentsection below. GitHub seems to be highlighting them strangely and code font is a little clearer anyway.

dbarnett · 2017-01-27T05:15:41Z

vimdoc/module.py

+    # We add a 'level' variable to locals so that WriteTableOfContents can keep
+    # track of the nesting.
+    def _AddChildSections(section):
+      if not 'level' in section.locals:


Could also use section.locals.setdefault('level', 0) here.

dbarnett · 2017-01-27T05:20:50Z

vimdoc/regex.py

@@ -211,7 +215,8 @@ def _DelimitedRegex(pattern):
  # MATCH GROUP 1: The Name
  (
    # Non-commas or escaped commas or escaped escapes.
-    (?:[^\\,]|\\.)+
+    # Must not end with a space.


Was this change leftover from the other approach, or valuable in itself?

valuable in itself - it prevents problems with trailing spaces being changed to dashes.

dbarnett · 2017-01-27T05:37:52Z

vimdoc/output.py

@@ -56,12 +56,22 @@ def WriteTableOfContents(self):
    """Writes the table of contents."""
    self.WriteRow()
    self.WriteLine('CONTENTS', right=self.Tag(self.Slug('contents')))
-    for i, block in enumerate(self.module.sections.values()):
+    # We need to keep track of section numbering on a per-level basis


Is it possible to extract out a generator function as a helper that tracks the indexing? Then the code in this method could look like:

for index, block in self._EnumerateIndices(self.module.sections.values()): self.WriteLine('%d. %s' % (index, block.locals['name']), indent=2 * block.locals['level'] + 1, right=self.Link(self.Slug(block.locals['id'])), fill='.')

nice idea. done.

dbarnett

Thanks for taking this task on!

We can bump the vimdoc version after this change and I can get an updated deb built after that. Then we can update vim-codefmt and vim-coverage to use it while wsdjeg is updating SpaceVim.

wsdjeg · 2017-01-28T06:48:25Z

Nice work, thanks a lot.

add a parent directive to @section

e7af55d

googlebot added the cla: yes label Jan 26, 2017

fix doctests

e16b610

dbarnett reviewed Jan 26, 2017

View reviewed changes

martindemello added 4 commits January 25, 2017 18:04

review fixes

c8d779f

better error message for missing parent section

0aaa5d0

revert allowing spaces before the comma in section names

ec7bffe

use a @parentsection directive rather than an arg to @section

a270fbc

dbarnett reviewed Jan 27, 2017

View reviewed changes

review fixes

23521ee

dbarnett approved these changes Jan 28, 2017

View reviewed changes

martindemello merged commit 5eb63f6 into google:master Jan 28, 2017

martindemello deleted the parent-section branch January 28, 2017 06:45

martindemello mentioned this pull request Jan 29, 2017

Add support for subsections #10

Closed

wsdjeg mentioned this pull request Jan 29, 2017

Use sub mode in doc SpaceVim/SpaceVim#175

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add a parent directive to @section #97

add a parent directive to @section #97

martindemello commented Jan 26, 2017 •

edited by dbarnett

Loading

dbarnett Jan 26, 2017

martindemello Jan 26, 2017

dbarnett Jan 26, 2017

martindemello Jan 26, 2017

dbarnett Jan 26, 2017

martindemello Jan 26, 2017

dbarnett Jan 26, 2017

dbarnett Jan 26, 2017

martindemello Jan 26, 2017

dbarnett Jan 26, 2017

martindemello Jan 26, 2017

dbarnett Jan 26, 2017

martindemello Jan 26, 2017

dbarnett left a comment

dbarnett Jan 27, 2017

martindemello Jan 27, 2017

dbarnett Jan 27, 2017

martindemello Jan 27, 2017

dbarnett Jan 27, 2017

martindemello Jan 27, 2017

dbarnett Jan 27, 2017

martindemello Jan 27, 2017

dbarnett Jan 27, 2017

martindemello Jan 27, 2017

dbarnett left a comment

wsdjeg commented Jan 28, 2017

add a parent directive to @section #97

add a parent directive to @section #97

Conversation

martindemello commented Jan 26, 2017 • edited by dbarnett Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dbarnett left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dbarnett left a comment

Choose a reason for hiding this comment

wsdjeg commented Jan 28, 2017

martindemello commented Jan 26, 2017 •

edited by dbarnett

Loading