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

mv: expose main functionality for nushell #5293

Closed
tertsdiepraam opened this issue Sep 20, 2023 · 4 comments · Fixed by #5335
Closed

mv: expose main functionality for nushell #5293

tertsdiepraam opened this issue Sep 20, 2023 · 4 comments · Fixed by #5335
Labels

Comments

@tertsdiepraam
Copy link
Member

Part of: nushell/nushell#10446

nushell wants access to our mv internals. Which means exposing the exec function and Behavior struct. Feel free to rename those to something more sensible for a public API (e.g. move & Options, to match the naming scheme from cp). There should be documentation for all the public items in the API as well.

This is an excellent first issue: feel free to claim this!

We did the same thing for cp already. You could use that PR as a guide: #5152

@KAAtheWiseGit
Copy link
Contributor

While rm refactoring is being discussed, I wanted to take on this issue.

I noticed mv uses a Behavior struct for options, whereas other commands use Options. Would it be fine to change its name to Options in the PR?

@tertsdiepraam
Copy link
Member Author

I think @PThorpe92 was working on this, but didn't mention that here yet. In the meantime, you could try to look at other utils. For example, the ones mentioned here: #5088 (comment). In general though, I think it's nice to make all utils as uniform as possible and I do encourage renaming as part of these PRs. Though more involved refactors should be done separately.

@PThorpe92
Copy link
Contributor

Yeah I'll have a PR in today on this.

@KAAtheWiseGit
Copy link
Contributor

Got it!

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

Successfully merging a pull request may close this issue.

3 participants