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

👌 Improve parsing of need option lists with dynamic functions #1272

Merged
merged 2 commits into from
Sep 5, 2024
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
219 changes: 73 additions & 146 deletions sphinx_needs/api/need.py
Original file line number Diff line number Diff line change
Expand Up @@ -164,6 +164,9 @@
type_style = ""
found = False

# location is used to provide source mapped warnings
location = (docname, lineno) if docname else None
ubmarco marked this conversation as resolved.
Show resolved Hide resolved

# Log messages for need elements that could not be imported.
configured_need_types = [ntype["directive"] for ntype in types]
if need_type not in configured_need_types:
Expand All @@ -172,7 +175,7 @@
f"Couldn't create need {id}. Reason: The need-type (i.e. `{need_type}`) is not set "
"in the project's 'need_types' configuration in conf.py.",
"add",
location=(docname, lineno) if docname else None,
location=location,
)

for ntype in types:
Expand Down Expand Up @@ -234,79 +237,26 @@
"by config value 'needs_statuses'."
)

if tags is None:
tags = []
if len(tags) > 0:
# tags should be a string, but it can also be already a list, which can be used.
if isinstance(tags, str):
tags = [tag.strip() for tag in re.split("[;,]", tags)]
new_tags = [] # Shall contain only valid tags
for i in range(len(tags)):
if len(tags[i]) == 0 or tags[i].isspace():
log_warning(
logger,
f"Scruffy tag definition found in need {need_id!r}. "
"Defined tag contains spaces only.",
"add",
location=(docname, lineno) if docname else None,
tags = _split_list_with_dyn_funcs(tags, location)
# Check if tag is in needs_tags. If not raise an error.
if needs_config.tags:
for tag in tags:
needs_tags = [tag["name"] for tag in needs_config.tags]
Copy link
Member

Choose a reason for hiding this comment

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

tag collides with outer tag variable (not a functional problem as this is like a lambda, however still fishy.
Another thing: needs_tags is generated for every tag in tags, however is never modified, so you can move it above the for loop.

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed
(note I would also like to replace these exceptions with warnings, but that's another PR)

if tag not in needs_tags:
raise NeedsTagNotAllowed(
f"Tag {tag} of need id {need_id} is not allowed "
"by config value 'needs_tags'."
)
else:
new_tags.append(tags[i])

tags = new_tags
# Check if tag is in needs_tags. If not raise an error.
if needs_config.tags:
for tag in tags:
needs_tags = [tag["name"] for tag in needs_config.tags]
if tag not in needs_tags:
raise NeedsTagNotAllowed(
f"Tag {tag} of need id {need_id} is not allowed "
"by config value 'needs_tags'."
)
# This may have cut also dynamic function strings, as they can contain , as well.
# So let put them together again
# ToDo: There may be a smart regex for the splitting. This would avoid this mess of code...
else:
tags = []
tags = _fix_list_dyn_func(tags)

if constraints is None:
constraints = []
if len(constraints) > 0:
# tags should be a string, but it can also be already a list,which can be used.
if isinstance(constraints, str):
constraints = [
constraint.strip() for constraint in re.split("[;,]", constraints)
]

new_constraints = [] # Shall contain only valid constraints
for i in range(len(constraints)):
if len(constraints[i]) == 0 or constraints[i].isspace():
log_warning(
logger,
f"Scruffy constraint definition found in need {need_id!r}. "
"Defined constraint contains spaces only.",
"add",
location=(docname, lineno) if docname else None,

constraints = _split_list_with_dyn_funcs(constraints, location)
# Check if constraint is in needs_constraints. If not raise an error.
if needs_config.constraints:
for constraint in constraints:
if constraint not in needs_config.constraints:
raise NeedsConstraintNotAllowed(
f"Constraint {constraint} of need id {need_id} is not allowed "
"by config value 'needs_constraints'."
)
else:
new_constraints.append(constraints[i])

constraints = new_constraints
# Check if constraint is in needs_constraints. If not raise an error.
if needs_config.constraints:
for constraint in constraints:
if constraint not in needs_config.constraints.keys():
raise NeedsConstraintNotAllowed(
f"Constraint {constraint} of need id {need_id} is not allowed "
"by config value 'needs_constraints'."
)
# This may have cut also dynamic function strings, as they can contain , as well.
# So let put them together again
# ToDo: There may be a smart regex for the splitting. This would avoid this mess of code...
else:
constraints = []
constraints = _fix_list_dyn_func(constraints)

#############################################################################################
# Add need to global need list
Expand All @@ -325,12 +275,7 @@
"from the content, then ensure the first sentence of the "
"requirements are different."
)
log_warning(
logger,
message,
"duplicate_id",
location=(docname, lineno) if docname else None,
)
log_warning(logger, message, "duplicate_id", location=location)
return []

# Trim title if it is too long
Expand Down Expand Up @@ -424,10 +369,12 @@
or len(str(kwargs[link_type["option"]])) == 0
):
# If it is in global option, value got already set during prior handling of them
links = _read_in_links(needs_info[link_type["option"]])
links = _split_list_with_dyn_funcs(

Check warning on line 372 in sphinx_needs/api/need.py

View check run for this annotation

Codecov / codecov/patch

sphinx_needs/api/need.py#L372

Added line #L372 was not covered by tests
needs_info[link_type["option"]], location
)
else:
# if it is set in kwargs, take this value and maybe override set value from global_options
links = _read_in_links(kwargs[link_type["option"]])
links = _split_list_with_dyn_funcs(kwargs[link_type["option"]], location)

needs_info[link_type["option"]] = links
needs_info["{}_back".format(link_type["option"])] = []
Expand Down Expand Up @@ -697,34 +644,6 @@
return new_content


def _read_in_links(links_string: None | str | list[str]) -> list[str]:
# Get links
links = []
if links_string:
# Check if links_string is really a string, otherwise it will be a list, which can be used
# without modifications
if isinstance(links_string, str):
link_list = re.split(";|,", links_string)
else:
link_list = links_string
for link in link_list:
if link.isspace():
log_warning(
logger,
f"Grubby link definition found in need {id}. "
"Defined link contains spaces only.",
None,
None,
)
else:
links.append(link.strip())

# This may have cut also dynamic function strings, as they can contain , as well.
# So let put them together again
# ToDo: There may be a smart regex for the splitting. This would avoid this mess of code...
return _fix_list_dyn_func(links)


def make_hashed_id(
app: Sphinx,
need_type: str,
Expand Down Expand Up @@ -770,48 +689,56 @@
return f"{type_prefix}{cal_hashed_id[:id_length]}"


def _fix_list_dyn_func(list: list[str]) -> list[str]:
"""
This searches a list for dynamic function fragments, which may have been cut by generic searches for ",|;".

Example:
`link_a, [[copy('links', need_id)]]` this will be splitted in list of 3 parts:

#. link_a
#. [[copy('links'
#. need_id)]]
def _split_list_with_dyn_funcs(
text: None | str | list[str], location: tuple[str, int | None] | None
) -> list[str]:
"""Split a ``;|,`` delimited string that may contain ``[[...]]`` dynamic functions.

This function fixes the above list to the following:
Remove any empty strings from the result.

#. link_a
#. [[copy('links', need_id)]]

:param list: list which may contain splitted function calls
:return: list of fixed elements
If the input is a list of strings, return the list unchanged,
or if the input is None, return an empty list.
"""
open_func_string = False
new_list = []
for element in list:
# If dyn_func got not cut, just add it
if "[[" in element and "]]" in element:
new_list.append(element)
# Other check if this is the starting element of dyn function
elif "[[" in element:
open_func_string = True
new_link = [element]
# Check if this is the ending element if dyn function
elif "]]" in element:
new_link.append(element)
open_func_string = False
element = ",".join(new_link)
new_list.append(element)
# Check it is a "middle" part of the dyn function
elif open_func_string:
new_link.append(element)
# Looks like it isn't a cut dyn_func, just add.
if text is None:
return []

if not isinstance(text, str):
assert isinstance(text, list) and all(
Copy link
Member

Choose a reason for hiding this comment

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

If this assertion raises it means a programming error in SN, right?
If not, just wondering, this will hard stop the build, right?

Copy link
Member Author

@chrisjsewell chrisjsewell Sep 5, 2024

Choose a reason for hiding this comment

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

yes it should be a programming error, so should be picked up in the test suite if failing

isinstance(x, str) for x in text
), "text must be a string or a list of strings"
return text

result: list[str] = []
_current_element = ""
while text:
if text.startswith("[["):
_current_element += text[:2]
text = text[2:]
while text and not text.startswith("]]"):
_current_element += text[0]
text = text[1:]
if _current_element.endswith("]"):
_current_element += "]"
else:
_current_element += "]]"
if not text.startswith("]]"):
log_warning(
ubmarco marked this conversation as resolved.
Show resolved Hide resolved
logger,
f"Dynamic function not closed correctly: {text}",
None,
location=location,
)
text = text[2:]
elif text[0] in ";|,":
result.append(_current_element)
_current_element = ""
text = text[1:]
else:
new_list.append(element)
return new_list
_current_element += text[0]
text = text[1:]
result.append(_current_element)
result = [element.strip() for element in result if element.strip()]
return result


def _merge_extra_options(
Expand Down
32 changes: 32 additions & 0 deletions tests/test_utils.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
"""Test utility functions."""

import pytest

from sphinx_needs.api.need import _split_list_with_dyn_funcs


@pytest.mark.parametrize(
"input, expected",
[
(None, []),
([], []),
(["a"], ["a"]),
("a", ["a"]),
("a,b", ["a", "b"]),
("a, b", ["a", "b"]),
("a,b,", ["a", "b"]),
("a|b", ["a", "b"]),
("a| b", ["a", "b"]),
("a|b,", ["a", "b"]),
("a;b", ["a", "b"]),
("a; b", ["a", "b"]),
("a;b,", ["a", "b"]),
("a,b|c;d,", ["a", "b", "c", "d"]),
("[[x,y]],b", ["[[x,y]]", "b"]),
("a,[[x,y]],b", ["a", "[[x,y]]", "b"]),
("a,[[x,y", ["a", "[[x,y]]"]),
("a,[[x,y]", ["a", "[[x,y]]"]),
],
)
def test_split_list_with_dyn_funcs(input, expected):
assert _split_list_with_dyn_funcs(input, None) == expected
Loading