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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow stor insert and stor update to accept pipeline input #12882

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

JoaoFidalgo1403
Copy link
Contributor

Description

This PR implements pipeline input support for the stor insert and stor update commands,
enabling users to directly pass data to these commands without relying solely on flag parameters.

Previously, it was only possible to specify the record data using flag parameters,
which could be less intuitive and become cumbersome:

stor insert --table-name nudb --data-record {bool1: true, int1: 5, float1: 1.1, str1: fdncred, datetime1: 2023-04-17}
stor update --table-name nudb --update-record {str1: nushell datetime1: 2020-04-17}

Now it is also possible to pass a record through pipeline input:

{bool1: true, int1: 5, float1: 1.1, str1: fdncred, datetime1: 2023-04-17} | stor insert --table-name nudb
{str1: nushell datetime1: 2020-04-17} | stor update --table-name nudb"

Changes made on code:

  • Modified stor insert and stor update to accept a record from the pipeline.
  • Added logic to handle data from the pipeline record.
  • Implemented an error case to prevent simultaneous data input from both pipeline and flag.

User-facing changes

Returns an error when both ways of inserting data are used.

The examples for both commands were updated and in each command, when the -d or -u fags are being used at the same time as input is being passed through the pipeline, it returns an error:

image

Also returns an error when both of them are missing:

image

Tests + Formating

  • 馃煝 toolkit fmt
  • 馃煝 toolkit clippy
  • 馃煝 toolkit test
  • 馃煝 toolkit test stdlib

This commit allows pipeline input support for the stor insert
and stor update commands, enabling users to directly pass data
to these commands without relying solely on flag parameters.

Changes made:
Modified stor insert and stor update to accept a record from
the pipeline.
Added logic to handle data from the pipeline record.
Implemented an error case to prevent simultaneous data input
from both pipeline and flag.

Previous usage:
stor insert --table-name nudb --data-record {bool1: true, int1: 5,
 float1: 1.1, str1: fdncred, datetime1: 2023-04-17}
stor update --table-name nudb --update-record {str1: nushell
 datetime1: 2020-04-17}

New usage with pipeline:
{bool1: true, int1: 5, float1: 1.1, str1: fdncred,
 datetime1: 2023-04-17} | stor insert --table-name nudb
{str1: nushell datetime1: 2020-04-17} | stor update
 --table-name nudb

This enhancement improves usability by allowing more flexible and
streamlined data input.

Co-authored-by: Jo茫o Fidalgo <joao.fidalgo.1403@tecnico.ulisboa.pt>
@fdncred
Copy link
Collaborator

fdncred commented May 16, 2024

There's already a PR for stor insert #12859

@JoaoFidalgo1403
Copy link
Contributor Author

JoaoFidalgo1403 commented May 16, 2024

We understand that there is already a PR addressing this issue. However, we wanted to highlight that we have also been working on this problem for some time. We believe our PR is comprehensive and well-organized. We deeply respect the work done by others, but we would be very grateful if you could consider reviewing our contribution as well.

@fdncred Thank you for your time and consideration.

@NotTheDr01ds
Copy link
Contributor

NotTheDr01ds commented May 21, 2024

Looking at the commit, this doesn't appear to handle a table type as an input, just a record. Is that correct? into sqlite can handle table or record as input types, and, as mentioned in #11433, it would be nice to have the same semantics for both into sqlite and stor insert. I was hoping we'd just be able to re-use and share the same code-base for all related commands.

@friaes
Copy link
Contributor

friaes commented May 23, 2024

We actually hadn't noticed that @NotTheDr01ds. Implementing that feature would indeed be more complex and challenging for us at the moment. However, we can definitely still make stor insert/update handle table as input in a future pull request, if that wouldn't be a problem.

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.

Accept pipeline input for stor insert values
4 participants