Skip to content

[feat] add variations create#152

Merged
chantalchung merged 4 commits into
mainfrom
DVC-7634-variations-create
Jun 14, 2023
Merged

[feat] add variations create#152
chantalchung merged 4 commits into
mainfrom
DVC-7634-variations-create

Conversation

@chantalchung

@chantalchung chantalchung commented Jun 12, 2023

Copy link
Copy Markdown
Contributor

TODO: add some tests! -- tested manually, will create ticket for tests

  • add variations create command
  • prompts for variables values (with proper types) if optional variables flag is not passed in
  • refactor cleanup command to use updated variable values prompt, which returns the correct types now
    • keep the cleanup command variable values prompt to use variableValueStringPrompt, as the types are handled in src/utils/refactor/RefactorEngine.ts

cli-variations-create

JSON variable:

cli-variations-create-json

@chantalchung
chantalchung requested a review from a team June 12, 2023 19:41
Comment thread src/commands/variations/create.ts Outdated
} else if (variable.type === 'Number') {
variablePrompts.push(variableValueNumberPrompt(variable.key))
} else {
variablePrompts.push(variableValueStringPrompt(variable.key))

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

How are JSON variables going to be handled here? Is the idea that they will be treated as strings?

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.

oh yeah I assumed that they were stringified and sent to the api that way? let me test it out

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.

I added a prompt for JSON variables. It looks like inquirer only returns string/numbers and not JSON objects? So it validates the input by doing JSON.parse, but the command that calls the prompt has to call JSON.parse on the returned string "answer" again.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

oh, so if the user passes in an invalid json object the inquirer prompt will catch that?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I was going to say that we may need to wrap the JSON.parse in a try/catch to handle for that, but if we can be sure that whatever comes back from the prompt is JSON parsable, then this should be fine

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.

@chantalchung
chantalchung force-pushed the DVC-7634-variations-create branch 2 times, most recently from da1b347 to 80db5ad Compare June 12, 2023 22:03
Comment thread src/api/variations.ts
import { Variation } from './schemas'
import { Feature, Variation } from './schemas'

export class CreateVariationParams {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I believe this class is no longer required given some work @emmawillis has been doing - zodios is supposed to auto-pickup all these validation decorators. But might need to check with her, or possibly she can update this on her ticket.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

ah yeah i haven't gotten to that yet unfortunately, so we're still using plainToClass to parse the input and the create params classes are still temporarily needed. whenever we get to this ticket https://taplytics.atlassian.net/browse/DVC-7720 we'll switch the input validation to zod and then we'll be able to scrap all the Create___Params classes

@chantalchung
chantalchung force-pushed the DVC-7634-variations-create branch from 80db5ad to 4cad95e Compare June 14, 2023 14:54
@chantalchung
chantalchung merged commit 79b9f15 into main Jun 14, 2023
@chantalchung
chantalchung deleted the DVC-7634-variations-create branch June 14, 2023 15:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants