SG-38756 Fix parent property of PathTemplate in case of optional key #1016

ClementHector · 2025-03-25T15:23:55Z

Context

The parent property of PathTemplate doesn't create a correct PathTemplate when optional keys are used. For example, if you create a template shots/{Sequence}[-{seq_num}]/{Shot}/{Step}/work, the parent property will return a templatePath without the optional [-{seq_num}] part.

Problem

When creating a PathTemplate with optional keys, the parent property omits these keys in the generated path. This can result in incorrect or incomplete paths.

Solution

To solve this issue, instead of passing self.definition to the PathTemplate constructor, we use self._repr_def, which contains the optional keys.

Testing

A test has been added to verify that the parent property now correctly includes optional keys in the generated PathTemplate.

…nition

…traction

…h expected value

julien-lang · 2025-04-10T16:57:59Z

Thanks for your contribution @ClementHector! We are going to review and test.

ClementHector · 2025-04-15T08:53:27Z

With pleasure @julien-lang , don't hesitate to ask me for corrections.

ClementHector · 2025-12-01T21:05:44Z

Hi @julien-lang, Did you have time to do a review?

Copilot

Pull request overview

This PR fixes a bug in the PathTemplate.parent property where optional keys (enclosed in square brackets) were being omitted from the parent path. The fix ensures that when a template like shots/{Sequence}[-{seq_num}]/{Shot}/{Step}/work has its parent calculated, the optional key structure is preserved.

Key changes:

Introduced a new _dirname() method to properly handle optional keys when extracting the directory portion of a template path
Modified the parent property to use _dirname() instead of os.path.dirname(self.definition)
Added comprehensive test coverage for the parent property with optional keys and a new dirname property test suite

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 6 comments.

File	Description
python/tank/template.py	Added `_dirname()` helper method and updated `parent` property to correctly preserve optional keys in parent templates
tests/core_tests/test_templatepath.py	Added test cases for `parent` property with optional keys and new `TestDirname` test class with multiple optional key scenarios
azure-pipelines.yml	Added mock package installation step to the CI pipeline

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2025-12-02T17:52:19Z

python/tank/template.py

+        Returns the directory part of the template.
+
+        :returns: String
+        """
+        dirname, basename = os.path.split(self._repr_def)
+        if "[" in basename and "]" not in basename:
+            return dirname.split("[")[0]
+        return dirname
+


The logic for handling optional keys in _dirname() is incomplete. If the basename contains a [ but no ], it splits by [ and takes only the first part, but this doesn't account for cases where the opening bracket is in the dirname portion. For example, with shots/{Sequence}/[{Shot}]-{Step}, after os.path.split, basename would be [{Shot}]-{Step} (containing both [ and ]), and the condition would be false, but the dirname shots/{Sequence} would be incorrect if it also had an unclosed bracket. Consider a more robust approach that properly handles nested or partial optional sections.

Suggested change

Returns the directory part of the template.

:returns: String

"""

dirname, basename = os.path.split(self._repr_def)

if "[" in basename and "]" not in basename:

return dirname.split("[")[0]

return dirname

Returns the directory part of the template, robustly handling optional sections.

:returns: String

"""

return self._split_path_outside_brackets(self._repr_def)

@staticmethod

def _split_path_outside_brackets(path):

"""

Splits the path at the last path separator that is outside any brackets.

Returns the directory part.

"""

bracket_depth = 0

last_sep_outside = -1

for i, c in enumerate(path):

if c == "[":

bracket_depth += 1

elif c == "]":

bracket_depth = max(0, bracket_depth - 1)

elif c == os.sep and bracket_depth == 0:

last_sep_outside = i

if last_sep_outside == -1:

return ""

return path[:last_sep_outside]

Copilot · 2025-12-02T17:52:19Z

tests/core_tests/test_templatepath.py

+        definition = "shots/{Sequence}[-{seq_num}]/{Shot}/{Step}/work"
+        parent_expected_definitions = [
+            "shots/{Sequence}-{seq_num}/{Shot}/{Step}",
+            "shots/{Sequence}/{Shot}/{Step}",
+        ]
+        parent_expected__repr_def = os.path.dirname(definition)


The test verifies _definitions and _repr_def but doesn't explain why both are necessary or what they represent. Add a comment explaining that _definitions contains all possible expanded forms (with and without optional keys) while _repr_def is the original representation-preserving form with optional syntax intact.

Copilot · 2025-12-02T17:52:19Z

tests/core_tests/test_templatepath.py

+
+    def test_dirname_with_optional(self):
+        definition = "shots/{Sequence}/{Shot}[/{Step}]"
+        template = TemplatePath(definition, {}, root_path=self.project_root)


The TestDirname test cases pass an empty dictionary for keys instead of self.keys. This may not fully test the interaction between the dirname property and key validation. Consider using self.keys consistently to ensure the tests validate behavior with actual key definitions, or add a comment explaining why empty keys are intentional for these specific tests.

Copilot · 2025-12-02T17:52:20Z

tests/core_tests/test_templatepath.py

+
+    def test_dirname_with_optional_word_and_key(self):
+        definition = "shots/{Sequence}/{Shot}/[abc_{Step}]"
+        template = TemplatePath(definition, {}, root_path=self.project_root)


The TestDirname test cases pass an empty dictionary for keys instead of self.keys. This may not fully test the interaction between the dirname property and key validation. Consider using self.keys consistently to ensure the tests validate behavior with actual key definitions, or add a comment explaining why empty keys are intentional for these specific tests.

Copilot · 2025-12-02T17:52:20Z

tests/core_tests/test_templatepath.py

+
+    def test_dirname_with_optional_key_and_key(self):
+        definition = "shots/{Sequence}/[{Shot}]-{Step}"
+        template = TemplatePath(definition, {}, root_path=self.project_root)


The TestDirname test cases pass an empty dictionary for keys instead of self.keys. This may not fully test the interaction between the dirname property and key validation. Consider using self.keys consistently to ensure the tests validate behavior with actual key definitions, or add a comment explaining why empty keys are intentional for these specific tests.

Copilot · 2025-12-02T17:52:20Z

python/tank/template.py

-        parent_definition = os.path.dirname(self.definition)
-        if parent_definition:
+        parent_definition = self._dirname()
+        if parent_definition and parent_definition != "/":


The check parent_definition != \"/\" is Unix-specific and may not work correctly on Windows where root paths use drive letters (e.g., C:\\). Consider using os.path.dirname(parent_definition) != parent_definition or a more platform-agnostic approach to detect root paths.

Suggested change

if parent_definition and parent_definition != "/":

if parent_definition and os.path.dirname(parent_definition) != parent_definition:

ClementHector added 6 commits March 19, 2025 15:47

Use self._repr_def to create the parent template instead of self.defi…

d896ad8

…nition

Add test for TemplatePath parent with optional key

6021bbd

Add _dirname method to TemplatePath for improved directory handling

0ae2f93

Add tests for dirname method in TemplatePath to validate directory ex…

ed752fc

…traction

Fix formatting in test_templatepath.py for consistency

c04d861

Fix dirname output in test_dirname_with_optional_word_and_key to matc…

3163f52

…h expected value

ClementHector marked this pull request as ready for review March 25, 2025 15:24

Update azure-pipelines.yml

1bf7819

julien-lang changed the title ~~Fix parent property of PathTemplate in case of optional key~~ SG-38756 Fix parent property of PathTemplate in case of optional key Apr 10, 2025

julien-lang requested a review from Copilot December 2, 2025 17:51

Copilot AI reviewed Dec 2, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

SG-38756 Fix parent property of PathTemplate in case of optional key #1016

SG-38756 Fix parent property of PathTemplate in case of optional key #1016

Uh oh!

ClementHector commented Mar 25, 2025

Uh oh!

julien-lang commented Apr 10, 2025

Uh oh!

ClementHector commented Apr 15, 2025

Uh oh!

ClementHector commented Dec 1, 2025

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Dec 2, 2025

Uh oh!

Copilot AI Dec 2, 2025

Uh oh!

Copilot AI Dec 2, 2025

Uh oh!

Copilot AI Dec 2, 2025

Uh oh!

Copilot AI Dec 2, 2025

Uh oh!

Copilot AI Dec 2, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

-        Returns the directory part of the template.
-        :returns: String
-        """
-        dirname, basename = os.path.split(self._repr_def)
-        if "[" in basename and "]" not in basename:
-            return dirname.split("[")[0]
-        return dirname
+        Returns the directory part of the template, robustly handling optional sections.
+        :returns: String
+        """
+        return self._split_path_outside_brackets(self._repr_def)
+    @staticmethod
+    def _split_path_outside_brackets(path):
+        """
+        Splits the path at the last path separator that is outside any brackets.
+        Returns the directory part.
+        """
+        bracket_depth = 0
+        last_sep_outside = -1
+        for i, c in enumerate(path):
+            if c == "[":
+                bracket_depth += 1
+            elif c == "]":
+                bracket_depth = max(0, bracket_depth - 1)
+            elif c == os.sep and bracket_depth == 0:
+                last_sep_outside = i
+        if last_sep_outside == -1:
+            return ""
+        return path[:last_sep_outside]

	if parent_definition and parent_definition != "/":
	if parent_definition and os.path.dirname(parent_definition) != parent_definition:

SG-38756 Fix parent property of PathTemplate in case of optional key #1016

Are you sure you want to change the base?

SG-38756 Fix parent property of PathTemplate in case of optional key #1016

Uh oh!

Conversation

ClementHector commented Mar 25, 2025

Context

Problem

Solution

Testing

Uh oh!

julien-lang commented Apr 10, 2025

Uh oh!

ClementHector commented Apr 15, 2025

Uh oh!

ClementHector commented Dec 1, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Dec 2, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 2, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 2, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 2, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 2, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 2, 2025

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants