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

PR: Cleanups of g.os_path wrappers and related methods. #3273

Merged
merged 3 commits into from
Apr 10, 2023

Conversation

edreamleo
Copy link
Member

@edreamleo edreamleo commented Apr 9, 2023

  • Remove non-standard path conventions from c.scanAtPathDirectives.
    It used a recent-removed setting.
  • Remove workarounds for an ancient Python bug.
  • Simplify wrappers in other ways.
  • Use g.os_path_normslashes in several places.

@edreamleo edreamleo added Code Re Leo's code PullRequest labels Apr 9, 2023
@edreamleo edreamleo added this to the 6.7.3 milestone Apr 9, 2023
@edreamleo edreamleo self-assigned this Apr 9, 2023
@edreamleo
Copy link
Member Author

@boltex @tbpassin Another round of cleanups. This round is much easier, but I won't merge without your comments.

Copy link
Contributor

@tbpassin tbpassin left a comment

Choose a reason for hiding this comment

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

This is going to take me some time. But I don't see where the path in an @path node is included. It's essential to include that path somewhere.

@edreamleo
Copy link
Member Author

@tbpassin Thanks for checking. I think the problem with @path happened in an earlier PR.

Copy link
Contributor

@boltex boltex left a comment

Choose a reason for hiding this comment

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

Those changes look good. And I'll make sure to mirror them in leojs as soon as they are officially merged, But i dont know enough about os.path module in python to know if those are really appropriate changes. I'll leave it to you and Thomas to give the final word on this.

@tbpassin
Copy link
Contributor

Those changes look good. And I'll make sure to mirror them in leojs as soon as they are officially merged, But i dont know enough about os.path module in python to know if those are really appropriate changes.

The os.path functions can be complicated in what they do. How will leojs be able to access the file system at all? Normally one can't do that from a browser.

@tbpassin
Copy link
Contributor

Note that we have to make sure the new path code is going to work with @rst nodes. I have several @rst trees that start with a @path node, like this:

- @path c:\Tom\devel\leo\docs
    - @rst project-X-docs
        - [more of the subtree]

It is essential that the paths work out right for these kinds of trees.

@boltex
Copy link
Contributor

boltex commented Apr 10, 2023

@tbpassin Good observation, but it's not a problem at all really: The leojs extension can be installed as-is both in vscode on a computer to edit in local drives, or, in the browser to edit repositories directly (on github, azure repos, etc..) but not local files. As I explain below:

When leojs runs in a computer's local installation of vscode, it has access to node's path module, which is somewhat equivalent to python's path module. (for working with local files on that computer as normal/typical usage) So no problem there for writing equivalent methods in typescript that match what the original Leo in python does! :)

When leojs runs in vscode for the browser it works ONLY on a virtual filesystems of github or azure (and others) online-repository directly. It provides a shiv (an equivalent API) for node's path module. So the same things can be done no problem.

I guess, if you (really) wanted to work with files from your computer, but with vscode in the browser, just push your working folder as a repo! :)

Félix

@edreamleo
Copy link
Member Author

edreamleo commented Apr 10, 2023

@boltex @tbpassin Thanks for your comments and approving review.

I agree with Thomas that more testing is needed, especially for the botched PR #3264 (it broke @path).

In fact, I think we should test devel for at least two weeks (after merging the last path-related PR) before releasing 6.7.3.

However, this PR is almost certainly safe. Besides removing a workaround for an ancient Python bug, this PR makes only obviously-correct substitutions.

I'm going to merge this PR now. It should form the base of the PR that fixes #3264.

@edreamleo edreamleo merged commit 1abefd4 into devel Apr 10, 2023
@edreamleo edreamleo deleted the ekr-clean-g.os_path branch April 10, 2023 11:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Code Re Leo's code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants