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 sort logic available outside sort-by #5893

Merged
merged 1 commit into from Jun 27, 2022
Merged

Conversation

rgwood
Copy link
Contributor

@rgwood rgwood commented Jun 27, 2022

Description

This PR moves sorting logic so it is accessible by consumers of the nu-command crate. It also creates 2 helper functions. This work is being done to enable interactive sorting of card results in Nana.

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.
  • [N/A] 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

@stormasm
Copy link
Contributor

@rgwood for now until you can figure out a better place for it I would put
sort_utils.rs in filters at the same level of sort_by.rs
instead of one level up in nu-command.

@rgwood
Copy link
Contributor Author

rgwood commented Jun 27, 2022

@stormasm 1 problem with that: the filters module is not public and would need to be made public.

@stormasm
Copy link
Contributor

stormasm commented Jun 27, 2022

oh !
that is a problem... bummer....
and making it public is not a good solution...
@jntrnr may have an idea as to a good spot...

this actually hearkens back to our conversation the other day on discord when we were discussing this idea of the melding of nana and nushell...

so we we need a place where the "rust nana code lives"
that nushell has access to as well....
kind of like another crate idea / but its probably too early for that...

so I guess the spot you have it is fine (for now)....

@stormasm
Copy link
Contributor

stormasm commented Jun 27, 2022

@rgwood so I read the discord channel after my previous message here and saw that @jntrnr said option 2 of nu-utils but then @fdncred had a great point about why that is not a good spot...

so since you are working a lot on the Nana code

I would probably plan on eventually when you see fit or a "good time"

to start up that new crate with some appropriate name for all future nana / nushell melding code....
and this sorting code eventually would be the first candidate for it...

@stormasm
Copy link
Contributor

stormasm commented Jun 27, 2022

@rgwood here is one name for a crate that is kind of obvious, not sure if you like it...

nu-nana

Not sure what other folks think...

@rgwood
Copy link
Contributor Author

rgwood commented Jun 27, 2022

@stormasm not sure about nu-nana; this sorting code is needed by the sort-by command as well as Nana. I think a new crate will probably need to be more general than that.

@stormasm
Copy link
Contributor

stormasm commented Jun 27, 2022

@rgwood
cool makes sense...

so we are looking for a crate name for code
that is used both by nushell and nana

or our melding code of both projects...

it will be fun to come up with a name that works...
at least we know one thing...
the sort_util.rs code should probably go in a new crate.
so finding the name will be interesting...

@fdncred
Copy link
Collaborator

fdncred commented Jun 27, 2022

I'd also add that Nana is kind of a codename too. We probably shouldn't embed it into code. I'm betting we eventually rename it.

@rgwood
Copy link
Contributor Author

rgwood commented Jun 27, 2022

I'll merge this today if there are no objections.

@fdncred
Copy link
Collaborator

fdncred commented Jun 27, 2022

lgtm

@rgwood rgwood merged commit 06d5a31 into nushell:main Jun 27, 2022
fennewald pushed a commit to fennewald/nushell that referenced this pull request Jun 27, 2022
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.

None yet

3 participants