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

Add functions to work with filesystem paths: expand_path , get_home, get_abspath #3057

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

Florents-Tselai
Copy link

This PR brings to the public interface three functions already available in util.h.

  • get_home
  • get_abspath
  • expand_path

Along with the already available env function, they can make jq play more nicely with the filesystem.

Here's what they look like.

image

@Florents-Tselai
Copy link
Author

I've chosen slightly different names than those in util.h.
Also I'm not entirely sure that expand_path is worth having.
If this looks reasonable, I'll get to add a couple of tests too.

@wader
Copy link
Member

wader commented Mar 3, 2024

The get_-prefix feel a bit redundant? maybe get_home -> hoemdir, get_abspath -> abspath?

Currently in function names "path" is used to refer to a path expression, could this make things confusing?

@nicowilliams how would this fit naming-wise with the other IO stuff you worked on?

@Florents-Tselai
Copy link
Author

The get_-prefix feel a bit redundant? maybe get_home -> hoemdir, get_abspath -> abspath?

Currently in function names "path" is used to refer to a path expression, could this make things confusing?

I'm for homedir and absdir

@nicowilliams how would this fit naming-wise with the other IO stuff you worked on?

src/builtin.c Outdated Show resolved Hide resolved
@emanuele6
Copy link
Member

Also those functions have 0 arguments (just the input), not 1.
The error messages should say /0, not /1.

@nicowilliams
Copy link
Contributor

How about some docs?

@wader
Copy link
Member

wader commented Mar 7, 2024

Some tests would be nice too

@itchyny
Copy link
Contributor

itchyny commented Mar 22, 2024

homedir is good, it simply represents what it returns. abspath looks it converts any relative paths to absolute paths regardless of the existence, but current implementation does not seem to work with non-existing path. The command realpath prints the absolute path regardless of existence, so worth discussing which it should behave. expand_path looks an ambiguous term; expanding the home dir of the current user is what it does now (os.path.expanduser in Python), but there are possibility of misinterpreted as expanding wildcards, or expanding embed variables, or something else called shell expansion.

@Florents-Tselai
Copy link
Author

Florents-Tselai commented Mar 24, 2024

Yeah, you're right @itchyny; from these, only home_dir has clear semantics. The rest can be a bit flaky.
How about ditching the rest, and keep realpath and set behavior similar to https://www.man7.org/linux/man-pages/man3/realpath.3.html ?

@itchyny
Copy link
Contributor

itchyny commented Mar 24, 2024

I'm thinking this can be archived if system() (#1614) is included. Long term feature request though.

@jemc
Copy link

jemc commented Apr 12, 2024

If my PR for the --sandbox flag (#3092) is merged first, then this PR should also be updated so that these new filesystem features are blocked by the --sandbox flag.

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

Successfully merging this pull request may close these issues.

6 participants