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 #5335

Merged
merged 2 commits into from Oct 7, 2023

Conversation

PThorpe92
Copy link
Contributor

@PThorpe92 PThorpe92 commented Sep 28, 2023

References #5293
@tertsdiepraam I did try to base if off the PR you mentioned in the issue.

The reason for the change of variable names (b -> opts) in non-public functions, is simply to match the new naming. If this is something that isn't desired, let me know.

Let me know of any additional changes you are looking for.

Also, everything did end up working out in our CI (eza), as well as further testing for uutils-term-grid. Was removing the Alignment field all that you were waiting on to publish? Although now that we know it will be a drop-in replacement, there certainly isn't a rush. Thanks, we definitely appreciate it.

EDIT: Had to fix clippy warning for failed CI anyway so those ;'s are just other random warnings I saw.

@github-actions
Copy link

GNU testsuite comparison:

Congrats! The gnu test tests/tail/truncate is no longer failing!

Copy link
Member

@tertsdiepraam tertsdiepraam left a comment

Choose a reason for hiding this comment

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

Great! Thanks! I have some small suggestions. You could also take a quick look at the cargo doc output, to see how it looks there and maybe fix up the formatting.

src/uu/mv/src/mv.rs Outdated Show resolved Hide resolved
src/uu/mv/src/mv.rs Outdated Show resolved Hide resolved
src/uu/mv/src/mv.rs Show resolved Hide resolved
Copy link
Member

@tertsdiepraam tertsdiepraam left a comment

Choose a reason for hiding this comment

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

I like it! Just one last request: could you put the clippy fixes in a separate commit?

@tertsdiepraam
Copy link
Member

tertsdiepraam commented Oct 6, 2023

Btw, I'm getting ready to publish term grid: uutils/uutils-term-grid#20. Hopefully it will be up today.

@cakebaker
Copy link
Contributor

I think you have to rebase because your branch contains code I removed a few days ago in #5347 and so the tests fail.

@github-actions
Copy link

github-actions bot commented Oct 6, 2023

GNU testsuite comparison:

GNU test failed: tests/mv/mv-n. tests/mv/mv-n is passing on 'main'. Maybe you have to rebase?

@PThorpe92
Copy link
Contributor Author

Ahh. yeah I didn't see that so i must have messed up the merge conflict. Rebased so it should pass now 👍

@tertsdiepraam big thanks :D

@cakebaker cakebaker merged commit 1038508 into uutils:main Oct 7, 2023
45 checks passed
@cakebaker
Copy link
Contributor

Thanks for your PR :)

@tertsdiepraam
Copy link
Member

Thanks @PThorpe92!

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.

mv: expose main functionality for nushell
3 participants