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 path replace-extension to stdlib-candidate #1002

Merged
merged 11 commits into from
Jan 4, 2025

Conversation

WindSoilder
Copy link
Contributor

@WindSoilder WindSoilder commented Dec 26, 2024

This pr is trying to add a useful custom command which is mentioned here: nushell/nushell#14144

I think it's useful to play with file extensions.


This pr also set up testing for candidates. In order to do this, I made some changes:

  1. ported nu-std/testing.nu under stdlib-candidate folder, and making some changes.
  2. run candidate tests in toolkit check pr command, to make sure the test is run in CI.
  3. including stdlib-candidate to NU_LIB_DIRS when running lint, so the tests can pass linter.

Changes in stdlib-candidate/testing.nu:

  1. remove std/log usage
  2. including stdlib-candidate path in run-test command

@fdncred
Copy link
Collaborator

fdncred commented Dec 26, 2024

I think we need tests for stdlib candidates please.

@NotTheDr01ds
Copy link
Contributor

@fdncred As part of the updated README, I included the following:

  • Include doc comments for definitions and parameters that can be used with help
  • Include tests

The last two (doc and tests) do not need to be in place at the time of the initial inclusion in stdlib-candidate, but are, of course, required in order to promote to std.

We can revise that if need be. My thoughts on this were that not everything we put in std-rfc is going to get promoted, so perhaps requiring tests at this stage is perhaps unnecessary?

@fdncred
Copy link
Collaborator

fdncred commented Dec 27, 2024

I think it's a mistake to not require tests for candidates.

@WindSoilder
Copy link
Contributor Author

OK, I think it's good to add tests for candidates.
I added them and update the pr description

@fdncred
Copy link
Collaborator

fdncred commented Dec 30, 2024

Thanks for all the changes. I personally think it's good to have these test mechanisms in here.

I think this command needs a bit of clarification. It claims to update the extension of files, which it can be argued that it does that. But it doesn't really change the extension of files, does it? If there are files on the file system, they will be unchanged. It seems to only change the text in a table that contains file names. So, the file names are never changed with this command, correct?

@WindSoilder
Copy link
Contributor Author

WindSoilder commented Dec 30, 2024

Yeah it doesn't change the file name locally. I updated the doc

@NotTheDr01ds
Copy link
Contributor

What do you think about naming this something like path replace-extension.

path extension sounds like it's retrieving the extension.

Also, as I mentioned in the original feature request, this doesn't appear to handle "complex" extensions like we do with path parse --extension. The common case, of course, is .tar.gz. That's probably a minor issue.

I still think that the following is so straightforward that I personally don't see much of a need for this as a std command.

'sample.json' | path parse | $"($in.stem).nuon"
# => sample.nuon

But I'll defer to others if they feel it's important.

@WindSoilder
Copy link
Contributor Author

WindSoilder commented Jan 1, 2025

TBH I still think a command is good for it. And path replace-extension can also handle list input.

If using string interpolation approach, when we meet a list, it's required to use each:

['sample.json', 'sample2.json'] | path parse | each {|it| $"($it.stem).nuon"}

Though it's not a big deal.

I have renamed the command.
I'd vote it to be nu-std, but I will obey the final decision.

@WindSoilder WindSoilder changed the title add path extension to stdlib-candidate add path replace-extension to stdlib-candidate Jan 1, 2025
@NotTheDr01ds
Copy link
Contributor

I say we land it, advertise it and ask for feedback via Showcase, and then evaluate in a month or so whether we want to promote it to std or not.

@NotTheDr01ds NotTheDr01ds merged commit 2dadab7 into nushell:main Jan 4, 2025
1 check passed
@132ikl
Copy link
Contributor

132ikl commented Jan 4, 2025

I just noticed that this should be in the std-rfc folder, not the root level stdlib-candidate folder

WindSoilder pushed a commit that referenced this pull request Jan 6, 2025
…nd with-parent (#1011)

Extends #1002. Renames `path replace-extension` to `path
with-extension`, in following with other languages
([Rust](https://doc.rust-lang.org/std/path/struct.Path.html#method.with_extension),
[Python](https://docs.python.org/3/library/pathlib.html#pathlib.PurePath.with_suffix)),
and adds `path with-stem` and `path with-parent`. Also moves the `path`
module into `std-rfc` so it can be used like `use std-rfc/path`.

Adds a private helper function, `with-field`, that `with-extension`,
`with-stem`, and `with-parent` can use. These can each be dead simple
functions, while giving users more options for path manipulation.

The motivation for separate `with-extension`, `with-stem`, and
`with-parent` functions, rather than a more general function like `path
with` is the following:
- `with-extension` has special behavior for stripping periods
- you can tab-complete `path with<TAB>` to immediately see all the
possible options
- you can't accidentally pass an invalid field to `path with` 
- there can be separate examples for `with-extension`, `with-stem`,
`with-parent` for only the relevant functionality
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.

4 participants