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

path_resolver.relative_path: use standard library that just works #2107

Closed
wants to merge 1 commit into from

Conversation

ztmr
Copy link

@ztmr ztmr commented Apr 2, 2017

path_resolver.relative_path does not work if the path of interest is not in the subtree of the reference directory.

Original function naively cut longest common path prefix.

NOTE: Can we afford requiring pathname library? Does it work properly with OPAL?

WARNING: unable to run tests due to dependency errors so cannot confirm the test is correct, please review/rerun.

path_resolver.relative_path does not work if the path
of interest is not in the subtree of the reference directory.

Original function naively cut longest common path prefix.

NOTE: Can we afford requiring pathname library?
      Does it work properly with OPAL?
@mojavelinux
Copy link
Member

path_resolver.relative_path does not work if the path of interest is not in the subtree of the reference directory.

If I remember correctly, that is actually by design. What I can't remember is whether that was a compliance issue with AsciiDoc Python or there was another reason for placing that restriction. I need to revisit my notes and document what's going on here.

Original function naively cut longest common path prefix.

Again, I don't think it's naive. I think it's intentional...either for reasons of security or reasons of best practice. I need to refresh my memory.

I would prefer not to use the Pathname library. In my experience, it's very slow and not well written. There have been numerous blogs I've come across challenging it's quality. That should help explain its intentional absence here.

@mojavelinux
Copy link
Member

In other words, PathResolver is meant to be a more robust replacement for Pathname.

@mojavelinux
Copy link
Member

I did some research and I agree this is a bug.

mojavelinux added a commit to mojavelinux/asciidoctor that referenced this pull request Jul 23, 2017
- return filename as relative path if filename doesn't share common root with base directory
- add test for scenario
@mojavelinux
Copy link
Member

I fixed this by simply checking to ensure the filename shares a common root with the base directory before truncating.

The main reason we don't use Pathname is because we don't need all that logic. Part of the contract of the method is that both paths are already expanded. So all we have to do is take away the base directory part if they share the same root. This is very much an internal method.

@mojavelinux
Copy link
Member

See #2335

mojavelinux added a commit to mojavelinux/asciidoctor that referenced this pull request Jul 23, 2017
- return filename as relative path if filename doesn't share common root with base directory
- add test for scenario
mojavelinux added a commit to mojavelinux/asciidoctor that referenced this pull request Jul 23, 2017
- return filename as relative path if filename doesn't share common root with base directory
- add test for scenario
mojavelinux added a commit to mojavelinux/asciidoctor that referenced this pull request Jul 23, 2017
- return filename as relative path if filename doesn't share common root with base directory
- add test for scenario
mojavelinux added a commit that referenced this pull request Jul 23, 2017
- return filename as relative path if filename doesn't share common root with base directory
- add test for scenario
@mojavelinux mojavelinux added this to the v1.5.6.1 milestone Jul 23, 2017
@mojavelinux mojavelinux self-assigned this Jul 23, 2017
@mojavelinux
Copy link
Member

I've improved this logic even further in 1.5.7.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants