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

Unify asy conditionals and variables data structure #2494

Closed
wants to merge 26 commits into from

Conversation

daved
Copy link
Contributor

@daved daved commented Apr 14, 2023

StoryDX-1661 As a user I can access the same variables in scripts as I can in conditionals

@github-actions github-actions bot changed the base branch from version/0-38-0-RC1 to version/0-39-0-RC1 April 14, 2023 01:32
@daved daved requested a review from Naatan April 20, 2023 00:29
Copy link
Member

@Naatan Naatan left a comment

Choose a reason for hiding this comment

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

Haven't done a full review as I feel like we can land this without any need for reflection. Figure we should address that before addressing the rest of the review.

pkg/projget/projget.go Show resolved Hide resolved
Comment on lines +125 to +128
Project *Project
OS *OS
Shell string
Mixin func() *Mixin
Copy link
Member

Choose a reason for hiding this comment

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

I'm not seeing why this needs reflection. Why can't we give these values enough information to identify and act on their type differences? eg. add an interface with IsMixing() for example.

projVars := vars.New(auth, vars.NewProject(pj), shell)
conditional := constraints.NewPrimeConditional(projVars)
project.RegisterConditional(conditional)
_ = project.RegisterStruct(projVars)
Copy link
Member

Choose a reason for hiding this comment

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

As discussed on call; we shouldn't need reflection to register the top level expanders. The cost of using reflection outweighs the value of having this behave in an automatic way where we can't accidentally introduce a top level conditional without also introducing a top level expander. Top level changes are so rare that it's overkill.

_ = project.RegisterStruct(projVars)
}

pj.SetUpdateCallback(registerProjectVars)
Copy link
Member

Choose a reason for hiding this comment

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

Ideally this shouldn't be necessary as we are using the prime version of project.Projectfile, so we should always have access to the latest values already.

Comment on lines +56 to +58
func NewProjectForTest(pjf *projectfile.Project) (*project.Project, error) {
return newProject(output.Get(), nil, "noshell", pjf)
}
Copy link
Member

Choose a reason for hiding this comment

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

Please move this to a *_test.go file.

@daved
Copy link
Contributor Author

daved commented Jun 20, 2023

Closing in favor of #2568.

@daved daved closed this Jun 20, 2023
@daved daved deleted the green/fix_cond_vars.DX-1661 branch June 20, 2023 17:50
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.

2 participants