-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Conversation
04612f7
to
3750278
Compare
GNU testsuite comparison:
|
There was a problem hiding this 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.
3750278
to
e0d8d81
Compare
There was a problem hiding this 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?
Btw, I'm getting ready to publish term grid: uutils/uutils-term-grid#20. Hopefully it will be up today. |
I think you have to rebase because your branch contains code I removed a few days ago in #5347 and so the tests fail. |
GNU testsuite comparison:
|
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 |
Thanks for your PR :) |
Thanks @PThorpe92! |
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.