Repository: ansible/ansible
Pull request: Adding two new os.path filters and its docs
Signed-off-by: Shivam Durgbuns <sdurgbun@redhat.com>

##### SUMMARY
<!--- Describe the change below, including rationale and design decisions -->
Adding two new filters based on os.path, specifically: `os.path.commonpath`, `os.path.normpath`
Fixes #78675 

##### ISSUE TYPE
<!--- Pick one below and delete the rest -->
- Feature Pull Request

##### COMPONENT NAME
<!--- Write the short name of the module, plugin, task or feature below -->
core




Problem statement:
More os.path filters
### Summary

Please add more filters based on os.path, specifically: `os.path.commonpath`, `os.path.normpath`. It is cheap, but would be handy in such tasks as validation, archive management, programmatic path generation. Can replace a lot of loops and regex filters.

### Issue Type

Feature Idea

### Component Name

core

### Additional Information

none

### Code of Conduct

- [X] I agree to follow the Ansible Code of Conduct

Hints from issue discussion:
Files identified in the description:
None

If these files are incorrect, please update the `component name` section of the description or use the `!component` bot command.

[click here for bot help](https://github.com/ansible/ansibullbot/blob/devel/ISSUE_HELP.md)
<!--- boilerplate: components_banner --->
I would like to work on this. On both the filters: os.path.commonpath, os.path.normpath

Commit message: Adding two new os.path filters and its docs

Signed-off-by: Shivam Durgbuns <sdurgbun@redhat.com>

Patch to review:
diff --git a/docs/docsite/rst/playbook_guide/playbooks_filters.rst b/docs/docsite/rst/playbook_guide/playbooks_filters.rst
index 7a4c7f47a6ce90..08dcfce8717e9f 100644
--- a/docs/docsite/rst/playbook_guide/playbooks_filters.rst
+++ b/docs/docsite/rst/playbook_guide/playbooks_filters.rst
@@ -2021,6 +2021,18 @@ To join one or more path components:
 
 .. versionadded:: 2.10
 
+.. code-block:: yaml+jinja
+
+    {{ path | commonpath }}
+
+To get Normalize path, eliminating double slashes, etc. (new in version 2.10):
+
+.. code-block:: yaml+jinja
+
+    {{ path | normpath }}
+
+To get the longest common sub-path, given a sequence of path names. (new in version 2.10):
+
 Manipulating strings
 ====================
 
diff --git a/lib/ansible/plugins/filter/core.py b/lib/ansible/plugins/filter/core.py
index 52a2cd108e1be7..963ad5854e6680 100644
--- a/lib/ansible/plugins/filter/core.py
+++ b/lib/ansible/plugins/filter/core.py
@@ -565,6 +565,16 @@ def path_join(paths):
         raise AnsibleFilterTypeError("|path_join expects string or sequence, got %s instead." % type(paths))
 
 
+def commonpath(paths):
+    '''
+    takes a list of paths and it returns the longest common path from the given list.
+    '''
+    if is_sequence(paths):
+        return os.path.commonpath(paths)
+    else:
+        raise AnsibleFilterTypeError("|path_join expects sequence, got %s instead." % type(paths))
+
+
 class FilterModule(object):
     ''' Ansible core jinja2 filters '''
 
@@ -600,6 +610,8 @@ def filters(self):
             'win_basename': partial(unicode_wrap, ntpath.basename),
             'win_dirname': partial(unicode_wrap, ntpath.dirname),
             'win_splitdrive': partial(unicode_wrap, ntpath.splitdrive),
+            'commonpath': commonpath,
+            'normpath': partial(unicode_wrap, os.path.normpath),
 
             # file glob
             'fileglob': fileglob,


Code review comments:
diff_hunk: @@ -565,6 +565,16 @@ def path_join(paths):
         raise AnsibleFilterTypeError("|path_join expects string or sequence, got %s instead." % type(paths))
 
 
+def commonpath(paths):
+    '''
+    takes a list of paths and it returns the longest common path from the given list.
+    '''
path: lib/ansible/plugins/filter/core.py
text: @user1:
I understand that this has been copied from the code above, but those aren't the best examples of writing docstrings, unfortunately.

Per [PEP 257](https://peps.python.org/pep-0257/), the first line should be an imperative-style active-voice sentence. Starting with a capital letter, ending with a trailing period, fitting in 72 chars. If it doesn't, maybe it's too wordy and some of it needs to elsewhere.
If there's important details to add, those should go into an extended description section in a separate paragraph, separated with one empty line after the title.

Here's one way of solving this:
```suggestion
    """Retrieve the longest common path from the given list.

    :param paths: A list of file system paths.
    :type paths: List[str]

    :returns: The longest common path.
    :rtype: str
    """
```

(the param definitions here follow Sphinx's markup declared @ https://www.sphinx-doc.org/en/master/usage/restructuredtext/domains.html#info-field-lists)

@author:
No problem, I thought we aren't using descriptive docstrings for the functions here, thus copied the same as above to keep the singularity in code. Will make this change.

diff_hunk: @@ -565,6 +565,16 @@ def path_join(paths):
         raise AnsibleFilterTypeError("|path_join expects string or sequence, got %s instead." % type(paths))
 
 
+def commonpath(paths):
+    '''
+    takes a list of paths and it returns the longest common path from the given list.
+    '''
+    if is_sequence(paths):
path: lib/ansible/plugins/filter/core.py
text: @user1:
It's best to reverse the check, turning it into a guard expression.
Doing
```diff
-    if is_sequence(paths):
+    if not is_sequence(paths):
+        raise ...
```
would allow to avoid indenting the positive case unnecessarily (as in, the else-block would be removed).

@author:
Understood, will make this change.

diff_hunk: @@ -600,6 +610,8 @@ def filters(self):
             'win_basename': partial(unicode_wrap, ntpath.basename),
             'win_dirname': partial(unicode_wrap, ntpath.dirname),
             'win_splitdrive': partial(unicode_wrap, ntpath.splitdrive),
+            'commonpath': commonpath,
+            'normpath': partial(unicode_wrap, os.path.normpath),
path: lib/ansible/plugins/filter/core.py
text: @user1:
Why is only one of these wrapped?

@author:
wrapped as in in the partial function? actually, for the `commonpath` we will need to provide a list of paths, and I can see that the `unicode_wrap` only returns a string as an output, thus I didn't wrap it.
@user1 Is there any better way to do that?