Port find command
#658
Port find command
#658
Conversation
|
looking good so far. looks like it's going to be an upgrade to the nushell find command. |
ghost
marked this pull request as ready for review
|
Thanks! What has been done with the |
ghost
marked this pull request as draft
|
How's it going on this pr @arthur-targaryen? I remember having some discussion on this topic on discord. |
@fdncred I didn't find the time to work on ot this week but I've planned to finish it tomorrow 🙂 |
|
Sounds good. Thanks for the help and the update. |
| Value::Binary { .. } => todo!(), | ||
| Value::CellPath { .. } => todo!(), |
There was a problem hiding this comment.
Not sure how those should be handled.
There was a problem hiding this comment.
I'm not sure. I'd have to look to see how nushell handles them. Was there anything useful there when you looked?
There was a problem hiding this comment.
In nushell find converts each element of the input stream to a trimmed, lowercase string using convert_to_string().trim().to_lowercase(). Then it uses str::contains with the rest parameters of the command.
I tried to mimic this behavior for Value::CellPath in engine-q (see 45c006f).
However, the convert_to_string() method on Primivite::Binary in nushell always return an empty string, so we need to decide if we choose to change it, and if so how should it be changed.
There was a problem hiding this comment.
@arthur-targaryen - I don't think we support it just yet, but if you want, I think it'd be fine to file an issue and go with the assumption that we'll add support in the future. It shouldn't directly impact the find work.
|
@arthur-targaryen - is this ready? or are there things missing that need to be added before it's ready? |
Following you reply to my comment, I just want to replace the |
|
@arthur-targaryen Awesome! Can you also clear out the merge conflicts? |
ghost
marked this pull request as ready for review
|
@arthur-targaryen I'll land this if we're ready. Are we? |
Yep we are, finally 🙂 |
|
Thanks for all your hard work on this!!! |
findcommand (Porting commands to engine-q list #242).Value::test_nothing().