From 9c73581e99f88832005b868fd2b6030cb0989e09 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Lukas=20M=C3=BCller?= <lukas.mueller94@gmail.com>
Date: Tue, 1 Aug 2023 01:32:46 +0200
Subject: [PATCH] fix: formatting of SequencePath and AlternativePath (#2504)

These path types were formatted without parentheses even if they
contained multiple elements, resulting in string representations that
did not accurately represent the path.

This change fixes the formatting so that the string representations are
enclosed in parentheses when necessary.

- Fixes <https://github.com/RDFLib/rdflib/issues/2503>.
---
 rdflib/paths.py   | 24 ++++++++++++++----------
 test/test_path.py | 27 ++++++++++++++++++++++++++-
 2 files changed, 40 insertions(+), 11 deletions(-)

diff --git a/rdflib/paths.py b/rdflib/paths.py
index df7136178..9f9538784 100644
--- a/rdflib/paths.py
+++ b/rdflib/paths.py
@@ -213,6 +213,15 @@
 ZeroOrOne = "?"
 
 
+def _n3(
+    arg: Union["URIRef", "Path"], namespace_manager: Optional["NamespaceManager"] = None
+) -> str:
+    # type error: Item "Path" of "Union[Path, URIRef]" has no attribute "n3"  [union-attr]
+    if isinstance(arg, (SequencePath, AlternativePath)) and len(arg.args) > 1:
+        return "(%s)" % arg.n3(namespace_manager)
+    return arg.n3(namespace_manager)  # type: ignore[union-attr]
+
+
 @total_ordering
 class Path:
     __or__: Callable[["Path", Union["URIRef", "Path"]], "AlternativePath"]
@@ -260,8 +269,7 @@ def __repr__(self) -> str:
         return "Path(~%s)" % (self.arg,)
 
     def n3(self, namespace_manager: Optional["NamespaceManager"] = None) -> str:
-        # type error: Item "Path" of "Union[Path, URIRef]" has no attribute "n3"  [union-attr]
-        return "^%s" % self.arg.n3(namespace_manager)  # type: ignore[union-attr]
+        return "^%s" % _n3(self.arg, namespace_manager)
 
 
 class SequencePath(Path):
@@ -318,8 +326,7 @@ def __repr__(self) -> str:
         return "Path(%s)" % " / ".join(str(x) for x in self.args)
 
     def n3(self, namespace_manager: Optional["NamespaceManager"] = None) -> str:
-        # type error: Item "Path" of "Union[Path, URIRef]" has no attribute "n3"  [union-attr]
-        return "/".join(a.n3(namespace_manager) for a in self.args)  # type: ignore[union-attr]
+        return "/".join(_n3(a, namespace_manager) for a in self.args)
 
 
 class AlternativePath(Path):
@@ -345,8 +352,7 @@ def __repr__(self) -> str:
         return "Path(%s)" % " | ".join(str(x) for x in self.args)
 
     def n3(self, namespace_manager: Optional["NamespaceManager"] = None) -> str:
-        # type error: Item "Path" of "Union[Path, URIRef]" has no attribute "n3"  [union-attr]
-        return "|".join(a.n3(namespace_manager) for a in self.args)  # type: ignore[union-attr]
+        return "|".join(_n3(a, namespace_manager) for a in self.args)
 
 
 class MulPath(Path):
@@ -470,8 +476,7 @@ def __repr__(self) -> str:
         return "Path(%s%s)" % (self.path, self.mod)
 
     def n3(self, namespace_manager: Optional["NamespaceManager"] = None) -> str:
-        # type error: Item "Path" of "Union[Path, URIRef]" has no attribute "n3"  [union-attr]
-        return "%s%s" % (self.path.n3(namespace_manager), self.mod)  # type: ignore[union-attr]
+        return "%s%s" % (_n3(self.path, namespace_manager), self.mod)
 
 
 class NegatedPath(Path):
@@ -505,8 +510,7 @@ def __repr__(self) -> str:
         return "Path(! %s)" % ",".join(str(x) for x in self.args)
 
     def n3(self, namespace_manager: Optional["NamespaceManager"] = None) -> str:
-        # type error: Item "Path" of "Union[Path, URIRef]" has no attribute "n3"  [union-attr]
-        return "!(%s)" % ("|".join(arg.n3(namespace_manager) for arg in self.args))  # type: ignore[union-attr]
+        return "!(%s)" % ("|".join(_n3(arg, namespace_manager) for arg in self.args))
 
 
 class PathList(list):
diff --git a/test/test_path.py b/test/test_path.py
index ad967849f..970405237 100644
--- a/test/test_path.py
+++ b/test/test_path.py
@@ -54,15 +54,40 @@
             "rdfs:subClassOf?",
         ),
         (
-            RDF.type / RDFS.subClassOf * "*",
+            RDF.type / MulPath(RDFS.subClassOf, "*"),
             f"<{RDF.type}>/<{RDFS.subClassOf}>*",
             "rdf:type/rdfs:subClassOf*",
         ),
+        (
+            RDF.type / ((SequencePath(RDFS.subClassOf)) * "*"),
+            f"<{RDF.type}>/<{RDFS.subClassOf}>*",
+            "rdf:type/rdfs:subClassOf*",
+        ),
+        (
+            RDF.type / RDFS.subClassOf * "*",
+            f"(<{RDF.type}>/<{RDFS.subClassOf}>)*",
+            "(rdf:type/rdfs:subClassOf)*",
+        ),
         (
             -(RDF.type | RDFS.subClassOf),
             f"!(<{RDF.type}>|<{RDFS.subClassOf}>)",
             "!(rdf:type|rdfs:subClassOf)",
         ),
+        (
+            -(RDF.type | ((SequencePath(RDFS.subClassOf)) * "*")),
+            f"!(<{RDF.type}>|<{RDFS.subClassOf}>*)",
+            "!(rdf:type|rdfs:subClassOf*)",
+        ),
+        (
+            SequencePath(RDFS.subClassOf),
+            f"<{RDFS.subClassOf}>",
+            "rdfs:subClassOf",
+        ),
+        (
+            AlternativePath(RDFS.subClassOf),
+            f"<{RDFS.subClassOf}>",
+            "rdfs:subClassOf",
+        ),
     ],
 )
 def test_paths_n3(