Repository: cogent3/cogent3
Pull request #2356 (2025-06-16 04:51:35)

Fixed newick formatting bug
## Summary by Sourcery

Overhaul the Newick string generation to fix formatting bugs and simplify logic, standardize parameter naming across TreeNode methods with deprecation warnings, and update tests and other modules to align with the revised API.

Bug Fixes:
- Fix semicolon placement and root node handling in Newick output
- Correct inclusion of internal node names, branch lengths, and escape quoting in Newick formatting

Enhancements:
- Refactor get_newick to a recursive implementation with type annotations
- Standardize method parameters (e.g., include_self, tips_only) and add deprecation warnings for legacy argument names

Tests:
- Update test suite to call get_newick with explicit flags and adjust expected Newick outputs

Chores:
- Update various modules to use new parameter names (get_node_names, get_tip_names, taxonomy root lookup, consensus clade extraction)


Patch:
diff --git a/src/cogent3/core/tree.py b/src/cogent3/core/tree.py
index 32b6fb702..e5767e8bb 100644
--- a/src/cogent3/core/tree.py
+++ b/src/cogent3/core/tree.py
@@ -75,6 +75,31 @@ def _copy_node(n):
     return result
 
 
+def _format_node_name(
+    node, with_node_names: bool, escape_name: bool, with_distances: bool
+) -> str:
+    """Helper function to format node name according to parameters"""
+    if not node.is_tip() and not with_node_names:
+        node_name = ""
+    else:
+        node_name = node.name or ""
+
+    if (
+        node_name
+        and escape_name
+        and not (node_name.startswith("'") and node_name.endswith("'"))
+    ):
+        if re.search("""[]['"(),:;_]""", node_name):
+            node_name = "'{}'".format(node_name.replace("'", "''"))
+        else:
+            node_name = node_name.replace(" ", "_")
+
+    if with_distances and (length := getattr(node, "length", None)) is not None:
+        node_name = f"{node_name}:{length}"
+
+    return node_name
+
+
 class TreeNode:
     """Store information about a tree node. Mutable.
 
@@ -327,7 +352,12 @@ class TreeNode:
         """Returns True if the current is a root, i.e. has no parent."""
         return self._parent is None
 
-    def traverse(self, self_before=True, self_after=False, include_self=True):
+    def traverse(
+        self,
+        self_before: bool = True,
+        self_after: bool = False,
+        include_self: bool = True,
+    ) -> typing_extensions.Iterator[typing_extensions.Self]:
         """Returns iterator over descendants. Iterative: safe for large trees.
 
         self_before includes each node before its descendants if True.
@@ -859,11 +889,11 @@ class TreeNode:
 
     def get_newick(
         self,
-        with_distances=False,
-        semicolon=True,
-        escape_name=True,
-        with_node_names=False,
-    ):
+        with_distances: bool = False,
+        semicolon: bool = True,
+        escape_name: bool = True,
+        with_node_names: bool = False,
+    ) -> str:
         """Return the newick string of node and its descendents
 
         Parameters
@@ -873,71 +903,57 @@ class TreeNode:
         semicolon
             end tree string with a semicolon
         escape_name
-            if any of these characters []'"(),
+            if any of these characters []'"() are within the
             nodes name, wrap the name in single quotes
         with_node_names
-            includes internal node names (except 'root')
+            includes internal node names
         """
-        result = ["("]
-        nodes_stack = [[self, len(self.children)]]
-        node_count = 1
+        # Stack contains tuples of (tree node, visit flag)
+        stack = [(self, False)]
+        node_results = {}  # results cache
 
-        while nodes_stack:
-            node_count += 1
-            # check the top node, any children left unvisited?
-            top = nodes_stack[-1]
-            top_node, num_unvisited_children = top
-            if num_unvisited_children:  # has any child unvisited
-                top[1] -= 1  # decrease the # of children unvisited
-                # - for order
-                next_child = top_node.children[-num_unvisited_children]
-                # pre-visit
-                if next_child.children:
-                    result.append("(")
-                nodes_stack.append([next_child, len(next_child.children)])
-            else:  # no unvisited children
-                nodes_stack.pop()
-                # post-visit
-                if top_node.children:
-                    result[-1] = ")"
-
-                if top_node.name_loaded or with_node_names:
-                    if top_node.name is None or (
-                        with_node_names and top_node.is_root()
-                    ):
-                        name = ""
-                    else:
-                        name = str(top_node.name)
-                        if escape_name and not (
-                            name.startswith("'") and name.endswith("'")
-                        ):
-                            if re.search("""[]['"(),:;_]""", name):
-                                name = "'{}'".format(name.replace("'", "''"))
-                            else:
-                                name = name.replace(" ", "_")
-                    result.append(name)
-
-                if (
-                    with_distances
-                    and (length := getattr(top_node, "length", None)) is not None
-                ):
-                    result[-1] = f"{result[-1]}:{length}"
-
-                result.append(",")
-
-        len_result = len(result)
-        if len_result == 2:  # single node no name
-            return ";" if semicolon else ""
-
-        if len_result == 3:  # single node with name
-            return f"{result[1]};" if semicolon else result[1]
-        if semicolon:
-            result[-1] = ";"
-        else:
-            result.pop(-1)
-        return "".join(result)
+        while stack:
+            node, visited = stack.pop()
 
-    def remove_node(self, target) -> bool:
+            if not visited:
+                # First visit - push back for processing after children
+                stack.append((node, True))
+                # add each child to the stack
+                stack.extend((child, False) for child in node.children)
+            else:
+                # children have been seen once
+                node_name = _format_node_name(
+                    node,
+                    with_node_names=with_node_names,
+                    escape_name=escape_name,
+                    with_distances=with_distances,
+                )
+
+                # for tips with parent, the typical case
+                if node.is_tip() and node.parent:
+                    node_results[id(node)] = node_name
+                    continue
+
+                # collecting children
+                # Build result for this node
+                if children_newick := [
+                    node_results[id(child)] for child in node.children
+                ]:
+                    result = f"({','.join(children_newick)}){node_name}"
+                else:
+                    result = node_name
+
+                node_results[id(node)] = result
+
+        # final result
+        final_result = node_results[id(self)]
+
+        if self.is_root() and semicolon:
+            final_result = f"{final_result};"
+
+        return final_result
+
+    def remove_node(self, target: typing_extensions.Self) -> bool:
         """Removes node by identity instead of value.
 
         Returns True if node was present, False otherwise.
@@ -1009,7 +1025,7 @@ class TreeNode:
         if clade:
             # get the list of names contained by join_edge
             for child in join_edge.children:
-                branch_names = child.get_node_names(includeself=1)
+                branch_names = child.get_node_names(include_self=True)
                 edge_names.extend(branch_names)
 
         return edge_names
@@ -1024,18 +1040,18 @@ class TreeNode:
 
     def _get_sub_tree(
         self,
-        included_names,
-        constructor=None,
-        keep_root=False,
-        tipsonly=False,
-    ):
+        included_names: list[str],
+        constructor: TreeNode | PhyloNode | None = None,
+        keep_root: bool = False,
+        tips_only: bool = False,
+    ) -> typing_extensions.Self | None:
         """An equivalent node with possibly fewer children, or None"""
 
         # Renumber autonamed edges
         if constructor is None:
             constructor = self._default_tree_constructor()
 
-        if (self.name in included_names and not tipsonly) or (
+        if (self.name in included_names and not tips_only) or (
             self.name in included_names and self.istip()
         ):
             return self.deepcopy(constructor=constructor)
@@ -1043,7 +1059,7 @@ class TreeNode:
         # internal nodes will be elminated this way
         children = []
         for child in self.children:
-            st = child._get_sub_tree(included_names, constructor, tipsonly=tipsonly)
+            st = child._get_sub_tree(included_names, constructor, tips_only=tips_only)
             children.append(st)
 
         children = [child for child in children if child is not None]
@@ -1090,13 +1106,18 @@ class TreeNode:
             result = constructor(self, tuple(children))
         return result
 
+    @c3warn.deprecated_args(
+        "2025.9",
+        "consistency between methods",
+        [("tipsonly", "tips_only")],
+    )
     def get_sub_tree(
         self,
-        name_list,
-        ignore_missing=False,
-        keep_root=False,
-        tipsonly=False,
-    ):
+        name_list: list[str],
+        ignore_missing: bool = False,
+        keep_root: bool = False,
+        tips_only: bool = False,
+    ) -> typing_extensions.Self:
         """A new instance of a sub tree that contains all the otus that are
         listed in name_list.
 
@@ -1110,11 +1131,11 @@ class TreeNode:
             ancestor of all nodes kept in the subtree. Root to tip distance is
             then (possibly) different from the original tree. If True, the root to
             tip distance remains constant, but root may only have one child node.
-        tipsonly
+        tips_only
             only tip names matching name_list are allowed
 
         """
-        edge_names = set(self.get_node_names(includeself=1, tipsonly=tipsonly))
+        edge_names = set(self.get_node_names(include_self=True, tips_only=tips_only))
         if not ignore_missing:
             # this may take a long time
             for name in name_list:
@@ -1122,7 +1143,9 @@ class TreeNode:
                     msg = f"edge {name!r} not found in tree"
                     raise ValueError(msg)
 
-        new_tree = self._get_sub_tree(name_list, keep_root=keep_root, tipsonly=tipsonly)
+        new_tree = self._get_sub_tree(
+            name_list, keep_root=keep_root, tips_only=tips_only
+        )
         if new_tree is None:
             msg = "no tree created in make sub tree"
             raise TreeError(msg)
@@ -1305,32 +1328,46 @@ class TreeNode:
         with atomic_write(filename, mode="wt") as outf:
             outf.writelines(data)
 
-    def get_node_names(self, includeself=True, tipsonly=False):
+    @c3warn.deprecated_args(
+        "2025.9",
+        "consistency between methods",
+        [("includeself", "include_self"), ("tipsonly", "tips_only")],
+    )
+    def get_node_names(
+        self, include_self: bool = True, tips_only: bool = False
+    ) -> list[str]:
         """Return a list of edges from this edge - may or may not include self.
         This node (or first connection) will be the first, and then they will
         be listed in the natural traverse order.
 
         Parameters
         ----------
-        includeself : bool
+        include_self : bool
             excludes self.name from the result
 
-        tipsonly : bool
+        tips_only : bool
             only tips returned
         """
-        if tipsonly:
+        if tips_only:
             nodes = self.traverse(self_before=False, self_after=False)
         else:
             nodes = list(self.traverse())
-            if not includeself:
+            if not include_self:
                 nodes.remove(self)
         return [node.name for node in nodes]
 
-    def get_tip_names(self, includeself=False):
+    @c3warn.deprecated_args(
+        "2025.9",
+        "consistency between methods",
+        [("includeself", "include_self"), ("tipsonly", "tips_only")],
+    )
+    def get_tip_names(self, include_self: bool = False) -> list[str]:
         """return the list of the names of all tips contained by this edge"""
-        return self.get_node_names(includeself, tipsonly=True)
+        return self.get_node_names(include_self=include_self, tips_only=True)
 
-    def get_edge_vector(self, include_root=True):
+    def get_edge_vector(
+        self, include_root: bool = True
+    ) -> list[typing_extensions.Self]:
         """Collect the list of edges in postfix order
 
         Parameters
@@ -1339,24 +1376,27 @@ class TreeNode:
             specifies whether root edge included
 
         """
-        if include_root:
-            result = list(self.traverse(False, True))
-        else:
-            result = [n for n in self.traverse(False, True) if not n.isroot()]
-        return result
+        return (
+            list(self.traverse(self_before=False, self_after=True))
+            if include_root
+            else [
+                n
+                for n in self.traverse(self_before=False, self_after=True)
+                if not n.isroot()
+            ]
+        )
 
-    def _get_node_matching_name(self, name):
-        """
-        find the edge with the name, or return None
+    def get_node_matching_name(self, name: str) -> typing_extensions.Self:
+        """find the edge with the name
+
+        Raises
+        -------
+        TreeError if no edge with the name is found
         """
         for node in self.traverse(self_before=True, self_after=False):
             if node.name == name:
-                return node
-        return None
-
-    def get_node_matching_name(self, name):
-        node = self._get_node_matching_name(name)
-        if node is None:
+                break
+        else:
             msg = f"No node named '{name}' in {self.get_tip_names()}"
             raise TreeError(msg)
         return node
diff --git a/src/cogent3/evolve/parameter_controller.py b/src/cogent3/evolve/parameter_controller.py
index 777fe3dab..e6efa6492 100644
--- a/src/cogent3/evolve/parameter_controller.py
+++ b/src/cogent3/evolve/parameter_controller.py
@@ -318,7 +318,7 @@ class _LikelihoodParameterController(_LF):
 
             edge_sets = [
                 {"edges": [n]}
-                for n in self.tree.get_node_names(includeself=False)
+                for n in self.tree.get_node_names(include_self=False)
                 if n not in exclude_edges
             ]
         elif type(edge_sets) == dict:
diff --git a/src/cogent3/parse/ncbi_taxonomy.py b/src/cogent3/parse/ncbi_taxonomy.py
index a23ce6472..0f9f541db 100644
--- a/src/cogent3/parse/ncbi_taxonomy.py
+++ b/src/cogent3/parse/ncbi_taxonomy.py
@@ -229,7 +229,7 @@ class NcbiTaxonomy:
                         )
                     deadbeats[t.ParentId] = t
         self.Deadbeats = deadbeats
-        self.Root = t.root()
+        self.Root = t.get_root()
 
     def __getitem__(self, item):
         """If item is int, returns taxon by id: otherwise, searches by name.
diff --git a/src/cogent3/phylo/consensus.py b/src/cogent3/phylo/consensus.py
index 599a8ac9d..1aed9633c 100644
--- a/src/cogent3/phylo/consensus.py
+++ b/src/cogent3/phylo/consensus.py
@@ -87,7 +87,7 @@ def weighted_rooted_majority_rule(weighted_trees, strict=False, attr="support"):
         total += weight
         edges = tree.get_edge_vector()
         for edge in edges:
-            tips = edge.get_tip_names(includeself=True)
+            tips = edge.get_tip_names(include_self=True)
             tips = frozenset(tips)
             if tips not in cladecounts:
                 cladecounts[tips] = 0


Test patch:
diff --git a/tests/test_cluster/test_UPGMA.py b/tests/test_cluster/test_UPGMA.py
index 32ada6ea5..089c461e2 100644
--- a/tests/test_cluster/test_UPGMA.py
+++ b/tests/test_cluster/test_UPGMA.py
@@ -108,10 +108,10 @@ class UPGMATests(TestCase):
         node_order = self.node_order
         node_order = condense_node_order(matrix, index, node_order)
         assert node_order[1] is None
-        assert node_order[0].__str__() == "(a:0.5,b:0.5);"
-        assert node_order[2].__str__() == "c;"
-        assert node_order[3].__str__() == "d;"
-        assert node_order[4].__str__() == "e;"
+        assert str(node_order[0]) == "(a:0.5,b:0.5);"
+        assert str(node_order[2]) == "c;"
+        assert str(node_order[3]) == "d;"
+        assert str(node_order[4]) == "e;"
 
     def test_upgma_cluster(self):
         """UPGMA_cluster clusters nodes based on info in a matrix with UPGMA"""
diff --git a/tests/test_core/test_tree.py b/tests/test_core/test_tree.py
index 0575495dd..56eaff15d 100644
--- a/tests/test_core/test_tree.py
+++ b/tests/test_core/test_tree.py
@@ -133,19 +133,23 @@ def test_str(empty_node, one_child, big_parent):
     """TreeNode str should give Newick-style representation"""
     # note: name suppressed if None
     assert str(empty_node) == ";"
-    assert str(one_child) == "(b)a;"
-    assert str(big_parent) == "(0,1,2,3,4,5,6,7,8,9)x;"
+    assert one_child.get_newick(with_node_names=True) == "(b)a;"
+    assert big_parent.get_newick(with_node_names=True) == "(0,1,2,3,4,5,6,7,8,9)x;"
     big_parent[-1].extend("abc")
-    assert str(big_parent) == "(0,1,2,3,4,5,6,7,8,(a,b,c)9)x;"
+    assert (
+        big_parent.get_newick(with_node_names=True) == "(0,1,2,3,4,5,6,7,8,(a,b,c)9)x;"
+    )
 
 
 def test_get_newick(empty_node, one_child, big_parent):
     """Should return Newick-style representation"""
     assert empty_node.get_newick() == ";"
-    assert one_child.get_newick() == "(b)a;"
-    assert big_parent.get_newick() == "(0,1,2,3,4,5,6,7,8,9)x;"
+    assert one_child.get_newick(with_node_names=True) == "(b)a;"
+    assert big_parent.get_newick(with_node_names=True) == "(0,1,2,3,4,5,6,7,8,9)x;"
     big_parent[-1].extend("abc")
-    assert big_parent.get_newick() == "(0,1,2,3,4,5,6,7,8,(a,b,c)9)x;"
+    assert (
+        big_parent.get_newick(with_node_names=True) == "(0,1,2,3,4,5,6,7,8,(a,b,c)9)x;"
+    )
 
 
 def test_to_dict():
@@ -154,7 +158,7 @@ def test_to_dict():
     got = tr.to_rich_dict()
     attrs = {"length": None}
     expect = {
-        "newick": "(a,b,(c,d)e1)",
+        "newick": "(a,b,(c,d)e1)root",
         "edge_attributes": {
             "a": attrs,
             "b": attrs,
@@ -221,20 +225,20 @@ def test_multifurcating():
     # can't break up easily... sorry 80char
     exp_str = "((a:1.0,(b:2.0,c:3.0):0.0)d:4.0,((e:5.0,(f:6.0,g:7.0):0.0)h:8.0,(i:9.0,(j:10.0,k:11.0):0.0)l:12.0):0.0)m:14.0;"
     obs = t.multifurcating(2)
-    assert obs.get_newick(with_distances=True) == exp_str
+    assert obs.get_newick(with_distances=True, with_node_names=True) == exp_str
     assert t.get_newick(with_distances=True) != obs.get_newick(with_distances=True)
 
     obs = t.multifurcating(2, 0.5)
     exp_str = "((a:1.0,(b:2.0,c:3.0):0.5)d:4.0,((e:5.0,(f:6.0,g:7.0):0.5)h:8.0,(i:9.0,(j:10.0,k:11.0):0.5)l:12.0):0.5)m:14.0;"
-    assert obs.get_newick(with_distances=True) == exp_str
+    assert obs.get_newick(with_distances=True, with_node_names=True) == exp_str
 
     t_str = "((a,b,c)d,(e,f,g)h,(i,j,k)l)m;"
     exp_str = "((a,(b,c))d,((e,(f,g))h,(i,(j,k))l))m;"
     t = DndParser(t_str, constructor=TreeNode)
     obs = t.multifurcating(2)
-    assert obs.get_newick(with_distances=True) == exp_str
+    assert obs.get_newick(with_distances=True, with_node_names=True) == exp_str
     obs = t.multifurcating(2, eps=10)  # no effect on TreeNode type
-    assert obs.get_newick(with_distances=True) == exp_str
+    assert obs.get_newick(with_distances=True, with_node_names=True) == exp_str
 
     with pytest.raises(TreeError):
         # TreeNode does not support multifurcating with n=1
@@ -592,8 +596,8 @@ def test_copy():
 
     assert t.get_newick() == t2.get_newick()
 
-    t_simple = TreeNode(["t"])
-    u_simple = TreeNode(["u"])
+    t_simple = TreeNode("t")
+    u_simple = TreeNode("u")
     t_simple.append(u_simple)
 
     assert str(t_simple.copy()) == str(t_simple.copy())
@@ -881,20 +885,22 @@ def test_ancestors(tree_nodes):
 def test_newick_with_labelled_nodes():
     """return newick with internal nodes labelled"""
     treestrings = (
-        "(a,b,(c,(d,e)));",
-        "(a:0.1,b:0.2,(c:0.3,(d:0.4,e:0.5):0.6):0.7);",
-        "(a,b,(c,(d,e)edge.0)edge.1);",
+        "(a,b,(c,(d,e)))",
+        "(a:0.1,b:0.2,(c:0.3,(d:0.4,e:0.5):0.6):0.7)",
+        "(a,b,(c,(d,e)edge.0)edge.1)",
     )
     expect = (
-        "(a,b,(c,(d,e)edge.0)edge.1);",
-        "(a:0.1,b:0.2,(c:0.3,(d:0.4,e:0.5)edge.0:0.6)edge.1:0.7);",
-        "(a,b,(c,(d,e)edge.0)edge.1);",
+        "(a,b,(c,(d,e)edge.0)edge.1)root",
+        "(a:0.1,b:0.2,(c:0.3,(d:0.4,e:0.5)edge.0:0.6)edge.1:0.7)root",
+        "(a,b,(c,(d,e)edge.0)edge.1)root",
     )
     for i, treestring in enumerate(treestrings):
         if i < 2:
             continue
         tree = make_tree(treestring=treestring)
-        nwk = tree.get_newick(with_node_names=True, with_distances=True)
+        nwk = tree.get_newick(
+            with_node_names=True, with_distances=True, semicolon=False
+        )
         assert nwk == expect[i]
 
 
@@ -1240,9 +1246,9 @@ def test_make_tree_array(tree_root):
 def test_get_node_names():
     """get_node_names works correctly"""
     tree = make_tree(treestring="((a:3,(b:2,(c:1,d:1):1):1):2,(e:3,f:3):2);")
-    names = tree.get_node_names(includeself=False, tipsonly=False)
+    names = tree.get_node_names(include_self=False, tips_only=False)
     assert tree.name not in names
-    names = tree.get_node_names(includeself=True, tipsonly=False)
+    names = tree.get_node_names(include_self=True, tips_only=False)
     assert tree.name in names
     tree.get_node_matching_name("a")
 
@@ -1313,7 +1319,7 @@ def test_prune():
     samename_bug = DndParser("((A,B)SAMENAME,((C,D)SAMENAME));")
     samename_bug.prune()
     exp_tree_str = "((A,B)SAMENAME,(C,D)SAMENAME);"
-    assert str(samename_bug) == exp_tree_str
+    assert samename_bug.get_newick(with_node_names=True) == exp_tree_str
 
 
 def test_get_node_matching_name(tree_root, tree_nodes):
@@ -1632,13 +1638,12 @@ def test_phylo_str(phylo_nodes):
     f = nodes["f"]
     h = nodes["h"]
 
-    assert str(h) == "h:2;"
-    assert str(f) == "(g:3)f:2;"
-    assert str(a) == "(((d:1,e:4,(g:3)f:2)c:3)b:0,h:2)a;"
+    assert str(f) == "(g:3):2"
+    assert str(a) == "(((d:1,e:4,(g:3):2):3):0,h:2);"
     # check that None isn't converted any more
     h.length = None
     c.length = None  # need to test both leaf and internal node
-    assert str(a) == "(((d:1,e:4,(g:3)f:2)c)b:0,h)a;"
+    assert str(a) == "(((d:1,e:4,(g:3):2)):0,h);"
 
 
 def test_get_max_tip_tip_distance(phylo_nodes, phylo_root):
@@ -1978,20 +1983,20 @@ def test_get_newick_2():
     orig = "((A:1.0,B:2.0)ab:3.0,((C:4.0,D:5.0)cd:6.0,E:7.0)cde:8.0)all;"
     unlen = "((A,B)ab,((C,D)cd,E)cde)all;"
     tree = _maketree(orig)
-    assert tree.get_newick(with_distances=1) == orig
-    assert tree.get_newick() == unlen
+    assert tree.get_newick(with_distances=True, with_node_names=True) == orig
+    assert tree.get_newick(with_node_names=True) == unlen
 
     tree.name = "a'l"
     ugly_name = "((A,B)ab,((C,D)cd,E)cde)a'l;"
     ugly_name_esc = "((A,B)ab,((C,D)cd,E)cde)'a''l';"
-    assert tree.get_newick(escape_name=True) == ugly_name_esc
-    assert tree.get_newick(escape_name=False) == ugly_name
+    assert tree.get_newick(escape_name=True, with_node_names=True) == ugly_name_esc
+    assert tree.get_newick(escape_name=False, with_node_names=True) == ugly_name
 
     tree.name = "'a l'"
     quoted_name = "((A,B)ab,((C,D)cd,E)cde)'a l';"
     quoted_name_esc = "((A,B)ab,((C,D)cd,E)cde)'a l';"
-    assert tree.get_newick(escape_name=True) == quoted_name_esc
-    assert tree.get_newick(escape_name=False) == quoted_name
+    assert tree.get_newick(escape_name=True, with_node_names=True) == quoted_name_esc
+    assert tree.get_newick(escape_name=False, with_node_names=True) == quoted_name
 
 
 def test_XML():
@@ -2040,7 +2045,7 @@ def test_params_merge():
         "length": 7,
         "beta": float(2 * 3 + 4 * 5) / (3 + 4),
     }
-    assert str(t.get_sub_tree(["b", "c", "xxx"], ignore_missing=True)) == "(b:7,c)root;"
+    assert str(t.get_sub_tree(["b", "c", "xxx"], ignore_missing=True)) == "(b:7,c);"
     with pytest.raises(ValueError):
         # should raise ValueError if a tip is not found
         t.get_sub_tree(["b", "c", "xxx"])
@@ -2337,7 +2342,7 @@ def test_load_tree_from_json(tmp_path, tree_fxt, request):
     tree.write(json_path)
     got = load_tree(json_path)
     assert isinstance(got, PhyloNode)
-    assert got.get_newick() == tree.get_newick(with_node_names=True)
+    assert got.get_newick() == tree.get_newick()
     assert got.get_node_names() == tree.get_node_names()
     # now try using non json suffix
     json_path = tmp_path / "tree.txt"
@@ -2367,14 +2372,14 @@ def test_getsubtree_5():
         if "Mus" in edge.name:
             edge.name = "Mus musculus"
 
-    sub1 = tree.get_sub_tree(names, tipsonly=False)
+    sub1 = tree.get_sub_tree(names, tips_only=False)
     assert tree.same_topology(sub1)
     expect = make_tree(
         treestring="(Homo_sapiens,Mus_musculus,"
         "(Canis_familiaris,Ornithorhynchus_anatinus))",
         underscore_unmunge=True,
     )
-    sub2 = tree.get_sub_tree(names, tipsonly=True)
+    sub2 = tree.get_sub_tree(names, tips_only=True)
     assert expect.same_topology(sub2)
 
 
@@ -2388,7 +2393,7 @@ def test_getsubtree_6():
     for edge in tree.postorder(include_self=False):
         edge.params["non-scalar"] = {edge.name: vals.get(edge.name, 99)}
 
-    sub1 = tree.get_sub_tree(names, tipsonly=False)
+    sub1 = tree.get_sub_tree(names, tips_only=False)
     assert set(tree.get_tip_names()), set("ABC")
     # the edge value for "non-scalar" should be same as original tree
     assert all(sub1.get_node_matching_name(name).params["non-scalar"] for name in names)
@@ -2541,7 +2546,7 @@ def test_parser():
     nasty = "( (A :1.0,'B (b)': 2) [com\nment]pair:3,'longer name''s':4)dash_ed;"
     nice = "((A:1.0,'B (b)':2.0)pair:3.0,'longer name''s':4.0)dash_ed;"
     tree = make_tree(treestring=nasty, underscore_unmunge=True)
-    tidied = tree.get_newick(with_distances=1)
+    tidied = tree.get_newick(with_distances=True, with_node_names=True)
     assert tidied == nice
     assert tree.get_node_matching_name("pair").params["other"] == ["com\nment"]
 
@@ -2666,6 +2671,13 @@ def test_rooted_with_tip():
     assert {c.name for c in rooted.children} == {tip_name, f"{tip_name}-root"}
 
 
+def tested_rooted_newick():
+    tree = make_tree(treestring="(A:0.006,B:0.0025,C:0.003)")
+    nt = tree.rooted("A")
+    got = str(nt.get_newick(with_distances=False))
+    assert got in {"(A,(B,C));", "((B,C),A);", "((C,B),A);"}
+
+
 @pytest.mark.parametrize("constructor", [PhyloNode, TreeNode])
 def test_rooted_with_internal(constructor):
     treestring = "((((Rhesus:0.02,HowlerMon:0.0487):0.0115,Orangutan:0.008)abcd:0.003,Human:0.006):0.00013,Gorilla:0.0025,Chimpanzee:0.003)"
diff --git a/tests/test_evolve/test_likelihood_function.py b/tests/test_evolve/test_likelihood_function.py
index cadd9ba32..d7462101d 100644
--- a/tests/test_evolve/test_likelihood_function.py
+++ b/tests/test_evolve/test_likelihood_function.py
@@ -933,7 +933,7 @@ class LikelihoodFunctionTests(TestCase):
         lf.optimise(max_evaluations=10, limit_action="ignore", show_progress=False)
         stats = lf.get_statistics()
         timehet_edge_names = {
-            n for n in self.tree.get_node_names(includeself=False) if n not in edges
+            n for n in self.tree.get_node_names(include_self=False) if n not in edges
         }
         for t in stats:
             if t.title == "edge locus params":
@@ -1019,7 +1019,7 @@ class LikelihoodFunctionTests(TestCase):
         lf = self.submodel.make_likelihood_function(self.tree)
         lf.set_time_heterogeneity(is_independent=True)
         got = lf.get_num_free_params()
-        assert got == nfp + len(lf.tree.get_node_names(includeself=False)) - 1
+        assert got == nfp + len(lf.tree.get_node_names(include_self=False)) - 1
 
         # we should be able to specify a set of edges that get treated as a block
         # if not specified, the edges are considered to be not-independent
diff --git a/tests/test_parse/test_tree.py b/tests/test_parse/test_tree.py
index 197d2b60f..bea25ba36 100644
--- a/tests/test_parse/test_tree.py
+++ b/tests/test_parse/test_tree.py
@@ -185,7 +185,7 @@ class DndParserTests(TestCase):
         assert len(t) == 2
         assert len(t[0]) == 0  # first child is terminal
         assert len(t[1]) == 2  # second child has two children
-        assert str(t) == "(abc:3.0,(def:4.0,ghi:5.0)jkl:6.0);"
+        assert str(t) == "(abc:3.0,(def:4.0,ghi:5.0):6.0);"
         info_dict = {}
         for node in t.traverse():
             info_dict[node.name] = node.length
@@ -203,7 +203,7 @@ class DndParserTests(TestCase):
         )
         tdata = DndParser(node_data_sample, unescape_name=True)
         assert (
-            str(tdata)
+            tdata.get_newick(with_distances=True, with_node_names=True)
             == "((xyz:0.28124,(def:0.24498,mno:0.03627)A:0.1771)B:0.0487,abc:0.05925,(ghi:0.06914,jkl:0.13776)C:0.09853);"
         )
 
@@ -254,21 +254,27 @@ class PhyloNodeTests(TestCase):
     def test_gops(self):
         """Basic PhyloNode operations should work as expected"""
         p = PhyloNode()
-        assert str(p) == ";"
+        assert p.get_newick(with_node_names=True) == ";"
         p.name = "abc"
-        assert str(p) == "abc;"
+        assert p.get_newick(with_node_names=True) == "abc;"
         p.length = 3
-        assert str(p) == "abc:3;"  # don't suppress branch from root
+        assert (
+            p.get_newick(with_node_names=True, with_distances=True) == "abc:3;"
+        )  # don't suppress branch from root
         q = PhyloNode()
         p.append(q)
-        assert str(p) == "()abc:3;"
+        assert p.get_newick(with_node_names=True, with_distances=True) == "()abc:3;"
         r = PhyloNode()
         q.append(r)
-        assert str(p) == "(())abc:3;"
+        assert p.get_newick(with_node_names=True, with_distances=True) == "(())abc:3;"
         r.name = "xyz"
-        assert str(p) == "((xyz))abc:3;"
+        assert (
+            p.get_newick(with_node_names=True, with_distances=True) == "((xyz))abc:3;"
+        )
         q.length = 2
-        assert str(p) == "((xyz):2)abc:3;"
+        assert (
+            p.get_newick(with_node_names=True, with_distances=True) == "((xyz):2)abc:3;"
+        )
 
 
 def test_make_tree_simple():