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

rm: implement iterator interface #5344

Closed
wants to merge 8 commits into from

Conversation

tertsdiepraam
Copy link
Member

@tertsdiepraam tertsdiepraam commented Oct 1, 2023

Part of nushell/nushell#10447 (comment)

This path makes an iterator interface for rm. The iterator is the Remover type, which yields an UResult<()> for every file. nushell can choose to how to print the errors.

To achieve that, I had to make every function return errors instead of printing them. This in turn lead to some big refactors for remove_file and remove_dir. I wish there was a better way to write an iterator like this, but I think it turned out alright. Maybe we can clean it up more in the future.

Oh and walkdir was only getting in the way. So I removed that too.

@github-actions

This comment was marked as outdated.

@tertsdiepraam
Copy link
Member Author

Looks like I got some tests to fix 😄

@github-actions

This comment was marked as outdated.

1 similar comment
@github-actions

This comment was marked as outdated.

@github-actions
Copy link

github-actions bot commented Oct 6, 2023

GNU testsuite comparison:

Congrats! The gnu test tests/rm/ir-1 is no longer failing!
GNU test failed: tests/pwd/pwd-long. tests/pwd/pwd-long is passing on 'main'. Maybe you have to rebase?
GNU test failed: tests/rm/deep-2. tests/rm/deep-2 is passing on 'main'. Maybe you have to rebase?
GNU test failed: tests/rm/inaccessible. tests/rm/inaccessible is passing on 'main'. Maybe you have to rebase?
GNU test failed: tests/rm/unread2. tests/rm/unread2 is passing on 'main'. Maybe you have to rebase?

@sylvestre
Copy link
Contributor

@tertsdiepraam sorry but it needs some rebasing

@fdncred
Copy link

fdncred commented Feb 23, 2024

any updates here?

@tertsdiepraam
Copy link
Member Author

@fdncred Oof, it's been a while. I might have some ideas to bring this forward. An iterator may not be the right approach here. The code was getting too complicated.

But, I think we have missed the easy solution here: calling remove multiple times with a single file. I think that's much easier for us to build and for nushell to use. If I recall correctly, that should be enough for nushells's usecase.

@tertsdiepraam
Copy link
Member Author

I'm gonna go with a 2 step plan here:

  1. Take some of the refactors I did and extract them into a separate PR.
  2. And then re-evaluate the error handling. Either with a channel or an iterator or a callback kind of thing.

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

Successfully merging this pull request may close these issues.

3 participants