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

#18 implement behavior to calculate parents from exclusively-relative paths #83

Merged

Conversation

Kataane
Copy link
Contributor

@Kataane Kataane commented Jul 20, 2024

Closes #18.

Pull Request: Define Parent for LocalPath(".") and Relative Paths

Problem Description:
Currently, new LocalPath(".").Parent (similarly for paths like .. or ../..) is not defined. This creates ambiguity and inconsistency in the behavior of the library.

Proposed Solution:
After researching how other libraries handle the parent of paths such as ".", "..", and "../..", we propose that these should return null. This approach aligns with the general expectations and behavior seen in other path-handling libraries.

Changes Made:

  • Implemented logic to return null for the parent of LocalPath("."), LocalPath(".."), and other relative paths.
  • Added corresponding unit tests to ensure consistent behavior and to prevent regressions in the future. These tests cover the mentioned cases as well as any arbitrary exclusively-relative path.

Rationale:
It's important to note that the decision to return null is debatable. Here are some examples of how different languages handle these cases:

Python:

from pathlib import Path
import os

# Pathlib
path = Path(".")
print(f"For the . parent is {path.parent}")  # For the . parent is .
path = Path("..")
print(f"For the .. parent is {path.parent}")  # For the .. parent is .
path = Path("../..")
print(f"For the ../.. parent is {path.parent}")  # For the ../.. parent is ..

# os.path
path = os.path.dirname(".")
print(f"For the . parent is {path}")  # For the . parent is 
path = os.path.dirname("..")
print(f"For the .. parent is {path}")  # For the .. parent is 
path = os.path.dirname("../..")
print(f"For the ../.. parent is {path}")  # For the ../.. parent is ..

Go:

package main

import (
    "fmt"
    "path"
)

func main() {
    // .
    // .
    // ..
    parent := path.Dir(".")
    fmt.Println(parent)
    parent2 := path.Dir("..")
    fmt.Println(parent2)
    parent3 := path.Dir("../..")
    fmt.Println(parent3)
}

Java:

import java.nio.file.Path;
import java.nio.file.Paths;

public class Main {
    public static void main(String[] args) {
        // Parent of current directory: null
        // Parent of parent directory: null
        // Parent directory of the file: ..
        Path currentDir = Paths.get(".");
        Path parentDir = currentDir.getParent();
        System.out.println("Parent of current directory: " + parentDir);

        Path parentPath = Paths.get("..");
        Path parentOfParent = parentPath.getParent();
        System.out.println("Parent of parent directory: " + parentOfParent);

        Path filePath = Paths.get("../..");
        Path fileParent = filePath.getParent();
        System.out.println("Parent directory of the file: " + fileParent);
    }
}

C#:

using System;
using System.IO;

public class Program
{
    public static void Main()
    {
        // All returns the current working folder

        DirectoryInfo dirInfo = new DirectoryInfo(".");
        var parentDir = dirInfo.Parent;
        Console.WriteLine(parentDir);

        DirectoryInfo dirInfo2 = new DirectoryInfo("..");
        var parentDir2 = dirInfo2.Parent;
        Console.WriteLine(parentDir2);

        DirectoryInfo dirInfo3 = new DirectoryInfo("../..");
        var parentDir3 = dirInfo3.Parent;
        Console.WriteLine(parentDir3);
    }
}

Rust:

use std::path::Path;

fn main() {
    let path = Path::new(".");
    println!("{:?}", path.parent()); // Some("")

    let path = Path::new("..");
    println!("{:?}", path.parent()); // Some("")

    let path = Path::new("../..");
    println!("{:?}", path.parent()); // Some("..")
}

If you look behind the trends, you can see that modern, trendy languages try to reclaim the solved path or treat it fairly with this kind of behaviour.

Bottom line, I definitely can't claim that returning null is a good idea

@ForNeVeR ForNeVeR self-assigned this Jul 21, 2024
@ForNeVeR ForNeVeR self-requested a review July 21, 2024 17:09
@ForNeVeR
Copy link
Owner

ForNeVeR commented Aug 11, 2024

Thanks for a thorough investigation! I agree that returning null sounds more sound than whatever some other libraries do.

Copy link
Owner

@ForNeVeR ForNeVeR left a comment

Choose a reason for hiding this comment

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

So far I have extracted the following checklist:

  • See what about the absolute path
  • Remove Random from tests
  • Document this behavior
  • Fix ResolveRelativePaths
    • Essentially, it detects if a path is a relative-only
    • Perhaps there's a better way of detecting it on a normalized path? Say, just check if it ends with {separator}..
    • Rename and move appropriately
  • Add a test case for foo that should resolve to .
  • Test paths like .../... — should work as before

We can discuss and you can fix some of these if you want, otherwise I'm planning to work on these items after the next release myself.

@ForNeVeR
Copy link
Owner

ForNeVeR commented Sep 22, 2024

After some thought, I decided that we should go with the following invariant: calling Parent should effectively be the same as adding to a path .. (and re-normalizing it afterwards): meaning that .Parent should be defined for any relative path.

This will create some nice properties: say, (absolutePath / relativePath).Parent will be (almost always, except very weird cases1) the same as absolutePath / relativePath.Parent.

I'll work on updating this PR.

Footnotes

  1. My eventual plan is to handle the weird cases as well, thus making these two examples to behave exactly the same always. I'll open an issue on the details of this later.

@ForNeVeR ForNeVeR force-pushed the #18-add-behavior-for-exclusively-relative-paths branch from 4a076bc to 63f8440 Compare September 22, 2024 18:39
@ForNeVeR ForNeVeR force-pushed the #18-add-behavior-for-exclusively-relative-paths branch from 01fe1d8 to 079747c Compare September 22, 2024 19:07
Copy link
Owner

@ForNeVeR ForNeVeR left a comment

Choose a reason for hiding this comment

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

Thanks for your investigation and initial implementation! I make sure to credit you properly in the release notes.

@ForNeVeR ForNeVeR merged commit 5f911af into ForNeVeR:main Sep 22, 2024
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement and check the behavior to calculate parents from exclusively-relative paths (., ..)
2 participants