Skip to content

Commit 589968a

Browse files
felixfonteinfelixfontein
authored
Lint array stubs (#173)
* Lint array stubs. * Always check option/return value names.
1 parent dc29608 commit 589968a

File tree

11 files changed

+459
-6
lines changed

11 files changed

+459
-6
lines changed

‎changelogs/fragments/173-lint-array-stubs.yml

+2
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,2 @@
1+
minor_changes:
2+
- "When linting collection plugin docs, make sure that array stubs ``[...]`` are used when referencing sub-options or sub-return values inside lists, and are not used outside lists and dictionaries (https://github.com/ansible-community/antsibull-docs/pull/173)."

‎src/antsibull_docs/lint_plugin_docs.py

+72-6
Original file line numberDiff line numberDiff line change
@@ -35,6 +35,7 @@
3535
)
3636
from .jinja2.environment import doc_environment
3737
from .lint_helpers import load_collection_info
38+
from .markup.semantic_helper import split_option_like_name
3839
from .plugin_docs import walk_plugin_docs_texts
3940
from .process_docs import (
4041
get_collection_contents,
@@ -261,6 +262,15 @@ def is_valid_return_value(
261262
)
262263

263264

265+
def _create_lookup(
266+
opt: dom.OptionNamePart | dom.ReturnValuePart, link: list[str]
267+
) -> str:
268+
lookup = _NAME_SEPARATOR.join(link)
269+
if opt.entrypoint is not None:
270+
lookup = f"{opt.entrypoint}{_ROLE_ENTRYPOINT_SEPARATOR}{lookup}"
271+
return lookup
272+
273+
264274
class _MarkupValidator:
265275
errors: list[str]
266276

@@ -301,31 +311,78 @@ def _validate_plugin_fqcn(
301311
self._report_disallowed_collection(part, plugin_fqcn, key)
302312
return False
303313

314+
def _validate_option_like_name(
315+
self,
316+
key: str,
317+
opt: dom.OptionNamePart | dom.ReturnValuePart,
318+
what: t.Literal["option", "return value"],
319+
):
320+
try:
321+
split_option_like_name(opt.name)
322+
except ValueError as exc:
323+
self.errors.append(
324+
f"{key}: {opt.source}: {what} name {opt.name!r} cannot be parsed: {exc}"
325+
)
326+
327+
def _validate_link(
328+
self,
329+
key: str,
330+
opt: dom.OptionNamePart | dom.ReturnValuePart,
331+
what: t.Literal["option", "return value"],
332+
lookup: t.Callable[[str], str | None],
333+
):
334+
try:
335+
name = split_option_like_name(opt.name)
336+
except ValueError:
337+
return
338+
link: list[str] = []
339+
for index, part in enumerate(name):
340+
link.append(part[0])
341+
lookup_value = _create_lookup(opt, link)
342+
part_type = lookup(lookup_value)
343+
if part_type == "list" and part[1] is None and index + 1 < len(name):
344+
self.errors.append(
345+
f"{key}: {opt.source}: {what} name {opt.name!r} refers to"
346+
f" list {'.'.join(link)} without `[]`"
347+
)
348+
if part_type not in ("list", "dict", "dictionary") and part[1] is not None:
349+
self.errors.append(
350+
f"{key}: {opt.source}: {what} name {opt.name!r} refers to"
351+
f" {'.'.join(link)} - which is neither list nor dictionary - with `[]`"
352+
)
353+
304354
def _validate_option_name(self, opt: dom.OptionNamePart, key: str) -> None:
355+
self._validate_option_like_name(key, opt, "option")
305356
plugin = opt.plugin
306357
if plugin is None:
307358
return
308359
if not self._validate_plugin_fqcn(opt, plugin.fqcn, plugin.type, key):
309360
return
310-
lookup = _NAME_SEPARATOR.join(opt.link)
311-
if opt.entrypoint is not None:
312-
lookup = f"{opt.entrypoint}{_ROLE_ENTRYPOINT_SEPARATOR}{lookup}"
361+
lookup = _create_lookup(opt, opt.link)
313362
if not self._name_collector.is_valid_option(plugin.fqcn, plugin.type, lookup):
314363
prefix = "" if plugin.type in ("role", "module") else " plugin"
315364
self.errors.append(
316365
f"{key}: {opt.source}: option name does not reference to an existing"
317366
f" option of the {plugin.type}{prefix} {plugin.fqcn}"
318367
)
368+
return
369+
self._validate_link(
370+
key,
371+
opt,
372+
"option",
373+
lambda lookup_: self._name_collector.get_option_type(
374+
plugin.fqcn, plugin.type, lookup_ # type: ignore[union-attr]
375+
),
376+
)
319377

320378
def _validate_return_value(self, rv: dom.ReturnValuePart, key: str) -> None:
379+
self._validate_option_like_name(key, rv, "return value")
321380
plugin = rv.plugin
322381
if plugin is None:
323382
return
324383
if not self._validate_plugin_fqcn(rv, plugin.fqcn, plugin.type, key):
325384
return
326-
lookup = _NAME_SEPARATOR.join(rv.link)
327-
if rv.entrypoint is not None:
328-
lookup = f"{rv.entrypoint}{_ROLE_ENTRYPOINT_SEPARATOR}{lookup}"
385+
lookup = _create_lookup(rv, rv.link)
329386
if not self._name_collector.is_valid_return_value(
330387
plugin.fqcn, plugin.type, lookup
331388
):
@@ -334,6 +391,15 @@ def _validate_return_value(self, rv: dom.ReturnValuePart, key: str) -> None:
334391
f"{key}: {rv.source}: return value name does not reference to an"
335392
f" existing return value of the {plugin.type}{prefix} {plugin.fqcn}"
336393
)
394+
return
395+
self._validate_link(
396+
key,
397+
rv,
398+
"return value",
399+
lambda lookup_: self._name_collector.get_return_value_type(
400+
plugin.fqcn, plugin.type, lookup_ # type: ignore[union-attr]
401+
),
402+
)
337403

338404
def _validate_module(self, module: dom.ModulePart, key: str) -> None:
339405
self._validate_plugin_fqcn(module, module.fqcn, "module", key)

‎src/antsibull_docs/markup/semantic_helper.py

+38
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@
1212
import re
1313

1414
_ARRAY_STUB_RE = re.compile(r"\[([^\]]*)\]")
15+
_ARRAY_STUB_SEP_START_RE = re.compile(r"[\[.]")
1516
_FQCN_TYPE_PREFIX_RE = re.compile(r"^([^.]+\.[^.]+\.[^#]+)#([a-z]+):(.*)$")
1617
_IGNORE_MARKER = "ignore:"
1718

@@ -85,3 +86,40 @@ def parse_return_value(
8586
"return value name",
8687
require_plugin=require_plugin,
8788
)
89+
90+
91+
def split_option_like_name(name: str) -> list[tuple[str, str | None]]:
92+
"""
93+
Given an option/return value name, splits it up into components separated by ``.``,
94+
and extracts array stubs ``[...]``.
95+
"""
96+
result: list[tuple[str, str | None]] = []
97+
index = 0
98+
length = len(name)
99+
while index < length:
100+
m = _ARRAY_STUB_SEP_START_RE.search(name, pos=index)
101+
if m is None:
102+
result.append((name[index:], None))
103+
break
104+
part = name[index : m.start(0)]
105+
appendix = None
106+
index = m.start(0)
107+
if name[index] == "[":
108+
next_index = name.find("]", index)
109+
if next_index < 0:
110+
raise ValueError(
111+
f'Found "[" without closing "]" at position {index + 1} of {name!r}'
112+
)
113+
appendix = name[index : next_index + 1]
114+
index = next_index + 1
115+
result.append((part, appendix))
116+
if index == length:
117+
break
118+
if name[index] == ".":
119+
index += 1
120+
continue
121+
raise ValueError(
122+
f'Expecting separator ".", but got "{name[index]!r}"'
123+
f" at position {index + 1} of {name!r}"
124+
)
125+
return result

‎tests/functional/ansible-doc-cache-all-others.json

+20
Original file line numberDiff line numberDiff line change
@@ -22851,6 +22851,16 @@
2285122851
"has_action": false,
2285222852
"module": "foo4",
2285322853
"options": {
22854+
"correct_array_stubs": {
22855+
"description": [
22856+
"O(ansible.builtin.iptables#module:tcp_flags.flags[])",
22857+
"O(ns2.col.bar#filter:foo)",
22858+
"O(ns2.col.bar#filter:foo[])",
22859+
"O(ext.col.foo#module:foo[baz].bar)",
22860+
"RV(ext.col.foo#module:baz)",
22861+
"RV(ext.col.foo#module:baz[ ])"
22862+
]
22863+
},
2285422864
"existing": {
2285522865
"description": [
2285622866
"M(ansible.builtin.service)",
@@ -22876,6 +22886,16 @@
2287622886
"O(ns.col2.foo2#module:subfoo.BaZ)"
2287722887
]
2287822888
},
22889+
"incorrect_array_stubs": {
22890+
"description": [
22891+
"O(ansible.builtin.file#module:state[])",
22892+
"RV(ansible.builtin.stat#module:stat[foo.bar].exists)",
22893+
"RV(ansible.builtin.stat#module:stat.exists[])",
22894+
"O(ns.col2.foo2#module:subfoo[)",
22895+
"RV(ns.col2.foo2#module:bar[])",
22896+
"O(ext.col.foo#module:foo.bar)"
22897+
]
22898+
},
2287922899
"not_existing": {
2288022900
"description": [
2288122901
"M(ansible.builtin.foobar)",

‎tests/functional/ansible-doc-cache-all.json

+20
Original file line numberDiff line numberDiff line change
@@ -22770,6 +22770,16 @@
2277022770
"has_action": false,
2277122771
"module": "foo4",
2277222772
"options": {
22773+
"correct_array_stubs": {
22774+
"description": [
22775+
"O(ansible.builtin.iptables#module:tcp_flags.flags[])",
22776+
"O(ns2.col.bar#filter:foo)",
22777+
"O(ns2.col.bar#filter:foo[])",
22778+
"O(ext.col.foo#module:foo[baz].bar)",
22779+
"RV(ext.col.foo#module:baz)",
22780+
"RV(ext.col.foo#module:baz[ ])"
22781+
]
22782+
},
2277322783
"existing": {
2277422784
"description": [
2277522785
"M(ansible.builtin.service)",
@@ -22795,6 +22805,16 @@
2279522805
"O(ns.col2.foo2#module:subfoo.BaZ)"
2279622806
]
2279722807
},
22808+
"incorrect_array_stubs": {
22809+
"description": [
22810+
"O(ansible.builtin.file#module:state[])",
22811+
"RV(ansible.builtin.stat#module:stat[foo.bar].exists)",
22812+
"RV(ansible.builtin.stat#module:stat.exists[])",
22813+
"O(ns.col2.foo2#module:subfoo[)",
22814+
"RV(ns.col2.foo2#module:bar[])",
22815+
"O(ext.col.foo#module:foo.bar)"
22816+
]
22817+
},
2279822818
"not_existing": {
2279922819
"description": [
2280022820
"M(ansible.builtin.foobar)",

‎tests/functional/ansible-doc-cache-ns.col2.json

+20
Original file line numberDiff line numberDiff line change
@@ -931,6 +931,16 @@
931931
"has_action": false,
932932
"module": "foo4",
933933
"options": {
934+
"correct_array_stubs": {
935+
"description": [
936+
"O(ansible.builtin.iptables#module:tcp_flags.flags[])",
937+
"O(ns2.col.bar#filter:foo)",
938+
"O(ns2.col.bar#filter:foo[])",
939+
"O(ext.col.foo#module:foo[baz].bar)",
940+
"RV(ext.col.foo#module:baz)",
941+
"RV(ext.col.foo#module:baz[ ])"
942+
]
943+
},
934944
"existing": {
935945
"description": [
936946
"M(ansible.builtin.service)",
@@ -956,6 +966,16 @@
956966
"O(ns.col2.foo2#module:subfoo.BaZ)"
957967
]
958968
},
969+
"incorrect_array_stubs": {
970+
"description": [
971+
"O(ansible.builtin.file#module:state[])",
972+
"RV(ansible.builtin.stat#module:stat[foo.bar].exists)",
973+
"RV(ansible.builtin.stat#module:stat.exists[])",
974+
"O(ns.col2.foo2#module:subfoo[)",
975+
"RV(ns.col2.foo2#module:bar[])",
976+
"O(ext.col.foo#module:foo.bar)"
977+
]
978+
},
959979
"not_existing": {
960980
"description": [
961981
"M(ansible.builtin.foobar)",

‎tests/functional/baseline-default/collections/ns/col2/foo4_module.rst

+88
Original file line numberDiff line numberDiff line change
@@ -90,6 +90,50 @@ Parameters
9090
* - Parameter
9191
- Comments
9292

93+
* - .. raw:: html
94+
95+
<div class="ansible-option-cell">
96+
<div class="ansibleOptionAnchor" id="parameter-correct_array_stubs"></div>
97+
98+
.. _ansible_collections.ns.col2.foo4_module__parameter-correct_array_stubs:
99+
100+
.. rst-class:: ansible-option-title
101+
102+
**correct_array_stubs**
103+
104+
.. raw:: html
105+
106+
<a class="ansibleOptionLink" href="#parameter-correct_array_stubs" title="Permalink to this option"></a>
107+
108+
.. rst-class:: ansible-option-type-line
109+
110+
:ansible-option-type:`string`
111+
112+
.. raw:: html
113+
114+
</div>
115+
116+
- .. raw:: html
117+
118+
<div class="ansible-option-cell">
119+
120+
\ :ansopt:`ansible.builtin.iptables#module:tcp\_flags.flags[]`\
121+
122+
\ :ansopt:`ns2.col.bar#filter:foo`\
123+
124+
\ :ansopt:`ns2.col.bar#filter:foo[]`\
125+
126+
\ :ansopt:`ext.col.foo#module:foo[baz].bar`\
127+
128+
\ :ansretval:`ext.col.foo#module:baz`\
129+
130+
\ :ansretval:`ext.col.foo#module:baz[ ]`\
131+
132+
133+
.. raw:: html
134+
135+
</div>
136+
93137
* - .. raw:: html
94138

95139
<div class="ansible-option-cell">
@@ -160,6 +204,50 @@ Parameters
160204
\ :ansopt:`ns.col2.foo2#module:subfoo.BaZ`\
161205

162206

207+
.. raw:: html
208+
209+
</div>
210+
211+
* - .. raw:: html
212+
213+
<div class="ansible-option-cell">
214+
<div class="ansibleOptionAnchor" id="parameter-incorrect_array_stubs"></div>
215+
216+
.. _ansible_collections.ns.col2.foo4_module__parameter-incorrect_array_stubs:
217+
218+
.. rst-class:: ansible-option-title
219+
220+
**incorrect_array_stubs**
221+
222+
.. raw:: html
223+
224+
<a class="ansibleOptionLink" href="#parameter-incorrect_array_stubs" title="Permalink to this option"></a>
225+
226+
.. rst-class:: ansible-option-type-line
227+
228+
:ansible-option-type:`string`
229+
230+
.. raw:: html
231+
232+
</div>
233+
234+
- .. raw:: html
235+
236+
<div class="ansible-option-cell">
237+
238+
\ :ansopt:`ansible.builtin.file#module:state[]`\
239+
240+
\ :ansretval:`ansible.builtin.stat#module:stat[foo.bar].exists`\
241+
242+
\ :ansretval:`ansible.builtin.stat#module:stat.exists[]`\
243+
244+
\ :ansopt:`ns.col2.foo2#module:subfoo[`\
245+
246+
\ :ansretval:`ns.col2.foo2#module:bar[]`\
247+
248+
\ :ansopt:`ext.col.foo#module:foo.bar`\
249+
250+
163251
.. raw:: html
164252

165253
</div>

0 commit comments

Comments
 (0)