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

Implement eval command to upsert command piping #299

Merged
merged 20 commits into from
Nov 7, 2023
Merged

Conversation

RichardAkman
Copy link
Contributor

@RichardAkman RichardAkman commented Oct 31, 2023

This PR extends the TS upsert command API to support piping of an eval command result to an upsert command.

  • The Upsert command now accepts a new property called pipe that is a result of an eval command. If provided, the eval result is merged together with the provided configs (which take priority over an eval result). Any key that does not belong to the schema received in the upsert command is filtered, same goes for any key that is a template in the provided schema.
  • The Eval and Export commands have been changed to align with the new pipe parameter by also having their relevant parameters renamed to pipe (breaking change), the affected code and docs have been adjusted accordingly.
  • Also fixes a discovered unrelated bug where upserting false as a value does not get validated due to an erroneous condition.

Notes:

  • This is a breaking change
  • Requires a follow up change to the CLI
  • Requires a follow up adjustment in the Python and Go SDKs

@RichardAkman RichardAkman added the feat New feature or request label Oct 31, 2023
@RichardAkman RichardAkman self-assigned this Oct 31, 2023
@RichardAkman RichardAkman requested review from rannn505 and RonConfigu and removed request for rannn505 October 31, 2023 10:34
@RichardAkman RichardAkman linked an issue Oct 31, 2023 that may be closed by this pull request
4 tasks
Copy link
Contributor

@RonConfigu RonConfigu left a comment

Choose a reason for hiding this comment

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

Beautiful 🥇
Please review my questions/comments

ts/packages/ts/src/commands/UpsertCommand.ts Show resolved Hide resolved
ts/packages/ts/src/commands/UpsertCommand.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@RonConfigu RonConfigu left a comment

Choose a reason for hiding this comment

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

LGTM 💯

@RichardAkman RichardAkman requested a review from a team as a code owner November 1, 2023 08:12
Copy link
Contributor

@rannn505 rannn505 left a comment

Choose a reason for hiding this comment

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

fix

errorScope,
);
}
if (_.isEmpty(configs) && _.isEmpty(pipe)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

why not sppliting it via wach one of the loops?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't understand the question. This condition is to skip the async init method in case there is nothing to upsert.

const upsertConfigs = _(configs)
.entries()
.map<Config>(([key, value]) => {
this.validateConfigValue({ key, value, skipDeclarationAndTemplateValidation: false });
Copy link
Contributor

Choose a reason for hiding this comment

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

I would convert the whole mapper function here

Copy link
Contributor

Choose a reason for hiding this comment

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

split the skip flag to each of the validation

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I split the validations. I don't understand what "convert the whole mapper function here" means.. please elaborate.

Copy link
Contributor

@rannn505 rannn505 left a comment

Choose a reason for hiding this comment

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

please see comments and fix

@rannn505 rannn505 merged commit d51342d into main Nov 7, 2023
6 of 7 checks passed
@rannn505 rannn505 deleted the 297-impl-upsert-pipe branch November 7, 2023 01:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement eval command to upsert command piping
3 participants