Unify asy conditionals and variables data structure #2568
Conversation
daved
force-pushed
the
green/fix_cond_vars-2.DX-1661
branch
from
c13b182
to
0c8f91c
Compare
| case "project": | ||
| return projectFile.Project, nil | ||
| case "lock": | ||
| return projectFile.Lock, nil |
There was a problem hiding this comment.
"project" shadows the field "Vars.Project", and "lock" (at the top level) will be difficult to keep updated using the introduced design. It would be nice to either drop these or include them within the Projvars struct.
There was a problem hiding this comment.
Sorry, I had to stop the review as I want to change too many things here to the point that I'd rather start over. There is too much complexity being introduced here. Yes, we want variable unification; but if the cost of that unification is significantly more complex code that needs to be maintained, we need to find a happy medium instead. Nothing wrong with the code itself, just that it does not feel like the right solution for the problem we're facing.
Compounding all of this is DX-1814 - which isn't a priority right now but ultimately will result in some significant changes in how we expose these variables.
I'm going to drop this story for now. We will probably come back to it at a later date but as of right now I feel like we've already burned more time on this than warranted, and we're not close enough to the finish line to keep it going.
End result is I just don't want to invest any more time on this right now. I'm sorry.
| } | ||
| sshell := subshell.New(cfg) | ||
|
|
||
| pj, err := project.NewWithVars(out, auth, sshell.Shell()) |
There was a problem hiding this comment.
Why do we need a separate constructor for vars? Is this notably slower or something? This feels very error prone as we will have instances of project initialized with vars and instances without, and we have to just know which is which.
I'd prefer we implement this in a way where this is always the case.
There was a problem hiding this comment.
This is done to not uproot existing usages that do not require Vars for this diff. I imagined a new story to cover making all project usages (more?) equal.
| Path() string | ||
| URL() string | ||
| } | ||
| func NewPrimeConditional(structure interface{}) *Conditional { |
There was a problem hiding this comment.
-
We're no longer using this as a prime property so naming this PrimeConditional is misleading. Also I don't think anything outside of the prime package should deal with the concept of primes.
-
It doesn't seem like you ever send this anything other than
*project.Vars, so why the interface? Passing it the typed vars may also allow us to get rid of more reflection. If the issue is import cycles; we can move vars to its own package. I had an issue with the nameprojvars, but that was about naming more than anything. -
This should just be part of
NewConditional()IMO. There is no scenario I'm aware of where we use conditionals without a project.
There was a problem hiding this comment.
This is old code written 2 years ago that I simplified.
|
I hear you, and accept this decision, and just want to voice that the complexity introduced is dependent on developer comfort rather than technical difficulty. It's about 200 lines introduced containing only basic reflection. One way we can get this same unification is something I've brought up in the past. That is to preprocess the references like $project.example into {{.Project.Example}} so that the reflection is performed by the template package logic rather than us duplicating it. The issue, again, is legacy dependence on global behavior. So the top-level "lock" and "project" values would be lost entirely (though, I think that is worth the loss). |
No description provided.