-
-
Notifications
You must be signed in to change notification settings - Fork 168
ENH: Added support for multiple functions+description in a See Also block #172
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
Changes from 5 commits
5ca74fe
bb11821
30ec43d
1670bbb
a6911fe
af7eada
a6a1b20
d65f917
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||
---|---|---|---|---|---|---|---|---|---|---|
|
@@ -236,10 +236,30 @@ def _parse_param_list(self, content): | |||||||||
|
||||||||||
return params | ||||||||||
|
||||||||||
_role = r":(?P<role>\w+):" | ||||||||||
_funcbacktick = r"`(?P<name>(?:~\w+\.)?[a-zA-Z0-9_.-]+)`" | ||||||||||
_funcplain = r"(?P<name2>[a-zA-Z0-9_.-]+)" | ||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. See comment above regarding original |
||||||||||
_funcname = r"(" + _role + _funcbacktick + r"|" + _funcplain + r")" | ||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Are we intentionally disallowing names in backticks not preceded by roles (i.e. using the default role)? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That was the effect of the original |
||||||||||
_funcnamenext = _funcname.replace('role', 'rolenext').replace('name', 'namenext') | ||||||||||
_description = r"(?P<description>\s*:(\s+(?P<desc>\S+.*))?)?\s*$" | ||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. (I think we'd like to make the space before the colon obligatory, for consistency with param lists, but I don't think the current code requires it) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I believe that the original |
||||||||||
_func_rgx = re.compile(r"^\s*" + _funcname + r"\s*", re.X) | ||||||||||
|
||||||||||
_line_rgx = re.compile( | ||||||||||
r"^\s*" | ||||||||||
+ r"(?P<allfuncs>" # group for all function names | ||||||||||
+ _funcname | ||||||||||
+ r"(?P<morefuncs>([,]\s+" | ||||||||||
+ _funcnamenext + r")*)" | ||||||||||
+ r")" # end of "allfuncs" | ||||||||||
|
||||||||||
+ r"(\s*,)?" # Some function lists have a trailing comma | ||||||||||
|
||||||||||
+ _description, | ||||||||||
re.X) | ||||||||||
|
||||||||||
_name_rgx = re.compile(r"^\s*(:(?P<role>\w+):" | ||||||||||
r"`(?P<name>(?:~\w+\.)?[a-zA-Z0-9_.-]+)`|" | ||||||||||
r" (?P<name2>[a-zA-Z0-9_.-]+))\s*", re.X) | ||||||||||
|
||||||||||
empty_description = '..' | ||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't get why this is a public attribugte There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It is (now) used in |
||||||||||
|
||||||||||
def _parse_see_also(self, content): | ||||||||||
""" | ||||||||||
func_name : Descriptive text | ||||||||||
|
@@ -252,48 +272,62 @@ def _parse_see_also(self, content): | |||||||||
|
||||||||||
def parse_item_name(text): | ||||||||||
"""Match ':role:`name`' or 'name'""" | ||||||||||
m = self._name_rgx.match(text) | ||||||||||
if m: | ||||||||||
g = m.groups() | ||||||||||
if g[1] is None: | ||||||||||
return g[3], None | ||||||||||
else: | ||||||||||
return g[2], g[1] | ||||||||||
raise ParseError("%s is not a item name" % text) | ||||||||||
m = self._func_rgx.match(text) | ||||||||||
if not m: | ||||||||||
raise ParseError("%s is not a item name" % text) | ||||||||||
role = m.groupdict().get('role') | ||||||||||
|
role = m.groupdict().get('role') | |
role = m.group('role') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
description = ml.group('desc')
will suffice here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be cleaner to have parse_item_name
return the end
than the match. especially if you're going to call it the inscrutable m2
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please remove this commented section
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why do we allow a tuple?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only allow lists.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -331,13 +331,15 @@ def _strip_blank_lines(s): | |
|
||
|
||
def line_by_line_compare(a, b): | ||
empty_description = '..' | ||
rgx = re.compile(r"^\s+" + re.escape(empty_description) + "$") | ||
|
||
|
||
a = textwrap.dedent(a) | ||
b = textwrap.dedent(b) | ||
a = [l.rstrip() for l in _strip_blank_lines(a).split('\n')] | ||
b = [l.rstrip() for l in _strip_blank_lines(b).split('\n')] | ||
a = [rgx.sub('', l.rstrip()) for l in _strip_blank_lines(a).split('\n')] | ||
b = [rgx.sub('', l.rstrip()) for l in _strip_blank_lines(b).split('\n')] | ||
assert all(x == y for x, y in zip(a, b)) | ||
|
||
|
||
def test_str(): | ||
# doc_txt has the order of Notes and See Also sections flipped. | ||
# This should be handled automatically, and so, one thing this test does | ||
|
@@ -709,36 +711,46 @@ def test_see_also(): | |
multiple lines | ||
func_f, func_g, :meth:`func_h`, func_j, | ||
func_k | ||
func_f1, func_g1, :meth:`func_h1`, func_j1 | ||
func_f2, func_g2, :meth:`func_h2`, func_j2 : description of multiple | ||
:obj:`baz.obj_q` | ||
:obj:`~baz.obj_r` | ||
:class:`class_j`: fubar | ||
foobar | ||
""") | ||
|
||
assert len(doc6['See Also']) == 13 | ||
for func, desc, role in doc6['See Also']: | ||
if func in ('func_a', 'func_b', 'func_c', 'func_f', | ||
'func_g', 'func_h', 'func_j', 'func_k', 'baz.obj_q', | ||
'~baz.obj_r'): | ||
assert(not desc) | ||
else: | ||
assert(desc) | ||
|
||
if func == 'func_h': | ||
assert role == 'meth' | ||
elif func == 'baz.obj_q' or func == '~baz.obj_r': | ||
assert role == 'obj' | ||
elif func == 'class_j': | ||
assert role == 'class' | ||
else: | ||
assert role is None | ||
|
||
if func == 'func_d': | ||
assert desc == ['some equivalent func'] | ||
elif func == 'foo.func_e': | ||
assert desc == ['some other func over', 'multiple lines'] | ||
elif func == 'class_j': | ||
assert desc == ['fubar', 'foobar'] | ||
assert len(doc6['See Also']) == 10 | ||
for funcs, desc in doc6['See Also']: | ||
for func, role in funcs: | ||
if func in ('func_a', 'func_b', 'func_c', 'func_f', | ||
'func_g', 'func_h', 'func_j', 'func_k', 'baz.obj_q', | ||
'func_f1', 'func_g1', 'func_h1', 'func_j1', | ||
'~baz.obj_r'): | ||
assert (not desc), str([func, desc]) | ||
rgommers marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
elif func in ('func_f2', 'func_g2', 'func_h2', 'func_j2'): | ||
assert (desc), str([func, desc]) | ||
|
||
else: | ||
assert(desc), str([func, desc]) | ||
|
||
|
||
if func == 'func_h': | ||
assert role == 'meth' | ||
elif func == 'baz.obj_q' or func == '~baz.obj_r': | ||
assert role == 'obj' | ||
elif func == 'class_j': | ||
assert role == 'class' | ||
elif func in ['func_h1', 'func_h2']: | ||
assert role == 'meth' | ||
else: | ||
assert role is None, str([func, role]) | ||
|
||
if func == 'func_d': | ||
assert desc == ['some equivalent func'] | ||
elif func == 'foo.func_e': | ||
assert desc == ['some other func over', 'multiple lines'] | ||
elif func == 'class_j': | ||
assert desc == ['fubar', 'foobar'] | ||
elif func in ['func_f2', 'func_g2', 'func_h2', 'func_j2']: | ||
assert desc == ['description of multiple'], str([desc, ['description of multiple']]) | ||
|
||
|
||
def test_see_also_parse_error(): | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's confusing to mix the use of
\w
and explicit character classes:Actually, I don't think the
~
or.
should be obligatory.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The regexes appearing in these patterns were taken from the original regex.
The patterns can be changed but I'd prefer that to be in a separate PR, since that will change the meaning.