ENB: For Thomas and Félix: more about PR #3277

[Edward K. Ream]

PR #3277 proposes changes to various path-related functions in leoGlobals.py.

These changes will affect leoJS. I don't expect them to expect Leo's users!

Thomas quite rightly raised some concerns. This Engineering Notebook post gives my response. Thomas's gadfly questions caused me to think more deeply, create better unit tests, and improve the related code.

Overview of the PR

The PR changes many files. Here are the highlights:

- Rename g.of_path_finalize to g.finalize, retaining the old name for compatibility.

- Rename g.os_path_finalize_join to g.finalize_join, retaining the old name for compatibility.

- Make g.os_path_expand_user a synonym for g.finalize.

- Slightly generalize g.finalize and g.finalize_join compared with legacy code.

Testing

test_g_finalize and test_g_finalize_join were responses to Thomas's compatibility concerns. I worked on these two tests for several days before realizing that these tests weren't the whole story!

The very first test case in test_g_finalize_join shows the problem. g.finalize('a.py') must return an absolute path relative to os.getcwd(), not the directory containing the outline! In other words, the callers of g.finalize_join are responsible for specifying the absolute path.

The PR lists all the places where arguments to these functions have changed. The PR's correctness depends on just those places.

Compatibility

Thomas suggested that better docstrings would help. I have just added improved docstrings for the two new functions. However, the code itself is likely easier to understand than the words.

I have given careful thought to the question of retaining the legacy code. I have chosen not to do so. There is no plausible scenario in which the legacy code could be superior to the new code.

Summary

Imo, Leo would be better off with the new code. This PR might cause compatibility problems, but the likelihood is small.

I plan to merge the new code in a few days, but I'll wait for Thomas and Félix to comment first. There is no rush. All comments are welcome.

Edward

[Edward K. Ream]

On Thursday, April 13, 2023 at 8:32:36 AM UTC-5 Edward K. Ream wrote:

> PR #3276...lists all the places where arguments to these functions have changed. The PR's correctness depends on just those places.

Leo's git-diff-pr command (run from the ekr-3267-simply-path branch) is the best way to see the changes. The c.fullPath and c.scanAtPathDirectives methods deserve close inspection. The new code does more with less cruft. The PR is likely sound if these two methods are sound.

Edward

[Thomas Passin]

On Thursday, April 13, 2023 at 9:32:36 AM UTC-4 Edward K. Ream wrote:

[...]

Testing

test_g_finalize and test_g_finalize_join were responses to Thomas's compatibility concerns. I worked on these two tests for several days before realizing that these tests weren't the whole story!

The very first test case in test_g_finalize_join shows the problem. g.finalize('a.py') must return an absolute path relative to os.getcwd(), not the directory containing the outline! In other words, the callers of g.finalize_join are responsible for specifying the absolute path.

I don't think that this agrees with my understanding of how things have worked in the past.  Please correct me if it's not so.

1. Paths in at-files should *not* depend on os.getcwd() because that value is state-dependent and is not necessarily defined only by the current outline.  The user may have run various scripts that changed it.  This could lead to changing and unpredictable locations for external files.

2.  When I create an external file like this, I expect its path to be relative to the *outline*:

@file special/good.py

3. When I use an @path node like this, I expect the at file to be located at a path which starts with the @path expression:

@path special

    @file good.py   # Path should be special/good.py relative to the outline.

4. Since the @path expression is to be prepended to the at-file path, if the @path is an absolute path, the at-file path should be absolute, not relative to the outline.

5. What should happen if *both* the @path and the @file contain absolute paths?  Of course, this shouldn't happen but what if it did?  I would say that the at-file's path should prevail, the @path should be disregarded, and an error message should be given on the first attempt to save the file.

IMO, 2) and 3) are **essential** for stability and sanity.  The changeset in devel as of yesterday afternoon behaves correctly with all the outlines containing external files I have opened.  I have not yet tested whether my @rst files go to the right place when I run the rst3 command.

[Thomas Passin]

On Thursday, April 13, 2023 at 10:13:11 AM UTC-4 Thomas Passin wrote:

On Thursday, April 13, 2023 at 9:32:36 AM UTC-4 Edward K. Ream wrote:

[...]

Testing

test_g_finalize and test_g_finalize_join were responses to Thomas's compatibility concerns. I worked on these two tests for several days before realizing that these tests weren't the whole story!

The very first test case in test_g_finalize_join shows the problem. g.finalize('a.py') must return an absolute path relative to os.getcwd(), not the directory containing the outline! In other words, the callers of g.finalize_join are responsible for specifying the absolute path.

I don't think that this agrees with my understanding of how things have worked in the past.  Please correct me if it's not so.

1. Paths in at-files should *not* depend on os.getcwd() because that value is state-dependent and is not necessarily defined only by the current outline.  The user may have run various scripts that changed it.  This could lead to changing and unpredictable locations for external files.

2.  When I create an external file like this, I expect its path to be relative to the *outline*:

@file special/good.py

3. When I use an @path node like this, I expect the at file to be located at a path which starts with the @path expression:

@path special

    @file good.py   # Path should be special/good.py relative to the outline.

4. Since the @path expression is to be prepended to the at-file path, if the @path is an absolute path, the at-file path should be absolute, not relative to the outline.

I didn't word this clearly enough.  I meant that the final path to the external file should be absolute because the @path segment was absolute.  The segment from the at-file node would be relative.

[Thomas Passin]

Or maybe I misunderstood and the references to os.cwd() only refer to the unit tests and not to how the paths are constructed when used in an outline?

[Edward K. Ream]

On Thu, Apr 13, 2023 at 10:07 AM Thomas Passin <tbp1...@gmail.com> wrote:

Or maybe I misunderstood and the references to os.cwd() only refer to the unit tests and not to how the paths are constructed when used in an outline?

Exactly. That's what confused me too.

That's why I said the unit tests aren't the whole story. The callers must specify a proper path. That's what c.fullPath does, and that's why the PR is sound only if c.fullPath is correct.

Edward