Skip to content

Return errors on unexpected inputs to take and first#7123

Merged
rgwood merged 3 commits intonushell:mainfrom
rgwood:fix-list-binary-in-first-and-take
Nov 13, 2022
Merged

Return errors on unexpected inputs to take and first#7123
rgwood merged 3 commits intonushell:mainfrom
rgwood:fix-list-binary-in-first-and-take

Conversation

@rgwood
Copy link
Copy Markdown
Contributor

@rgwood rgwood commented Nov 13, 2022

Previously, first and take were very loosey-goosey about the types of data they would accept. If given a type that isn't really iterable, they would often just return the input as-is (sometimes wrapping it in a list):

image

I believe this behaviour was accidental, an artifact of how take and first were implemented. I've done a refactoring to make take and first more robust when given unexpected input types:

image

This has the nice side effect of fixing #7121. The PR partially addresses #6941.

Future Work

Maybe these commands should handle strings by essentially performing a substring operation? That would be consistent with how they operate on binary values.

@dandavison
Copy link
Copy Markdown
Contributor

Maybe these commands should handle strings by essentially performing a substring operation?

Ah, I didn't realize that we treat strings as iterable, but

〉"nushell" | each {|char| $char | str upcase}
NUSHELL

so, yes, I think either strings are iterable and they behave mostly like lists of characters including with first and take, or strings are not iterable (and we get rid of anything like that each behavior above implying otherwise)

@rgwood
Copy link
Copy Markdown
Contributor Author

rgwood commented Nov 13, 2022

@dandavison I think I come down on the side of "strings are iterable", but I would prefer to leave that for another PR. For this PR I mostly wanted to clean up the obviously wrong behaviour.

@dandavison
Copy link
Copy Markdown
Contributor

Incidentally (not blocking this PR) I see other commands accepting non-iterable input, e.g.

 〉1 | where {$in == 2}
 〉1 | where {$in == 1}
╭───╮
│ 1 │
╰───╯

@rgwood
Copy link
Copy Markdown
Contributor Author

rgwood commented Nov 13, 2022

Yeah, webbedspace has a big list: #6941

Comment on lines -127 to -129
// if we want more rows that the current chunk size (8192)
// we must gradually get bigger chunks while testing
// if it's within the requested rows_desired size
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

For posterity: I think this code was left over from an earlier time and no longer necessary. I tested the new functionality and it is able to get byte ranges larger than 8192 bytes.

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.

3 participants