Skip to content
This repository has been archived by the owner on Sep 28, 2021. It is now read-only.

Add support for variables #305

Merged
merged 13 commits into from
Jun 4, 2019
Merged

Add support for variables #305

merged 13 commits into from
Jun 4, 2019

Conversation

glooms
Copy link
Contributor

@glooms glooms commented Jun 3, 2019

This PR contains commands to manage variables. It seems that there are two types of variables: Variables and GenericVariables. As we want to use GenericVariables - I am assuming - we have to use the method GetVariableByName as it is the only getter that returns said type.

Tests (golden even) pass, don't seem to have the same problems as bookmarks.

Closes #298

Copy link
Member

@wennmo wennmo left a comment

Choose a reason for hiding this comment

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

Looks good, just some minor comments. Can you also add the bash completion for variables to this PR? Otherwise it might be forgotten.

cmd/variable.go Outdated
Args: cobra.ExactArgs(1),
Short: "Evaluate the layout of an generic variable",
Long: "Evaluate the layout of an generic variable",
Example: "corectl variable layout DIMENSION-ID",
Copy link
Member

Choose a reason for hiding this comment

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

VARIABLE-ID

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Woops

@@ -60,7 +60,7 @@ var listDimensionsCmd = &cobra.Command{
Run: func(ccmd *cobra.Command, args []string) {
state := internal.PrepareEngineState(rootCtx, headers, false)
items := internal.ListDimensions(state.Ctx, state.Doc)
printer.PrintNamedItemsList(items, viper.GetBool("bash"))
printer.PrintNamedItemsList(items, viper.GetBool("bash"), false)
Copy link
Member

Choose a reason for hiding this comment

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

Do we only want to print the title for variables?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

With the bash-flag yes, since we can only retrieve variables by name. Normally the print for bash completion only prints IDs so it had to be modified to support this case.

Copy link
Member

@wennmo wennmo left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@glooms glooms merged commit f2fca48 into master Jun 4, 2019
@glooms glooms deleted the variables branch June 4, 2019 12:28
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add variables
3 participants