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

Make seq return a ListStream where possible. #6910

Closed
wants to merge 6 commits into from

Conversation

merelymyself
Copy link
Contributor

Description

Significant perf improvements as well: try something like seq 1 100000000000 and observe how it is streaming correctly.

Tests

Make sure you've done the following:

  • Add tests that cover your changes, either in the command examples, the crate/tests folder, or in the /tests folder.
  • Try to think about corner cases and various ways how your changes could break. Cover them with tests.
  • If adding tests is not possible, please document in the PR body a minimal example with steps on how to reproduce so one can verify your change works.

Make sure you've run and fixed any issues with these commands:

  • cargo fmt --all -- --check to check standard code formatting (cargo fmt --all applies these changes)
  • cargo clippy --workspace --features=extra -- -D warnings -D clippy::unwrap_used -A clippy::needless_collect to check that you're using the standard code style
  • cargo test --workspace --features=extra to check that all the tests pass

Documentation

Copy link
Member

@sholderbach sholderbach left a comment

Choose a reason for hiding this comment

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

That sounds like a worthwhile improvement, especially as in most cases you don't actually care about the small value being emitted.

But I think for correctness reasons, we should change the implementation to use the most appropriate numeric type. Ints when the bounds are ints and floats if float intervals are requested.

crates/nu-command/src/generators/seq.rs Outdated Show resolved Hide resolved
crates/nu-command/src/generators/seq.rs Outdated Show resolved Hide resolved
@merelymyself merelymyself marked this pull request as draft October 26, 2022 09:45
@merelymyself merelymyself marked this pull request as ready for review October 26, 2022 12:42
@@ -324,74 +357,110 @@ fn print_seq(
pad: bool,
padding: usize,
span: Span,
is_float_sequence: bool,
Copy link
Member

Choose a reason for hiding this comment

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

Mhh, the data is still coming in as float and may have lost precision. Could we just pass in the Counter?

@fdncred
Copy link
Collaborator

fdncred commented Nov 5, 2022

Hey @merelymyself, do you have time to continue working on this one? Seems like you've been busy recently and haven't got back to it.

@sholderbach
Copy link
Member

This has been kind of overtaken by #7045, but should now be easier to make consistently lazy.

@sholderbach sholderbach marked this pull request as draft November 17, 2022 22:23
@merelymyself
Copy link
Contributor Author

Hello! Sorry for the long period of inactivity, something cropped up in my life. Because this PR has been overtaken, I've opened a new one at #7367 and according will close this one. Thanks!

@fdncred
Copy link
Collaborator

fdncred commented Dec 6, 2022

Hello! Sorry for the long period of inactivity...

Hey! It's @merelymyself!!! It's so glad to see you back here. Welcome again!

@rgwood rgwood added the streaming Issues related to streaming data (or collecting data when it should be streamed) label Dec 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
streaming Issues related to streaming data (or collecting data when it should be streamed)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants