lang/funcs: Experimental "defaults" function #26766
Conversation
Codecov Report
|
apparentlymart
force-pushed
the
f-experimental-funcs
branch
from
fddb56a to
5be8a0e
Compare
apparentlymart
force-pushed
the
f-experimental-funcs
branch
from
5be8a0e to
4877dbc
Compare
apparentlymart
force-pushed
the
f-experimental-funcs
branch
from
4877dbc to
41f1acc
Compare
apparentlymart
force-pushed
the
f-experimental-funcs
branch
from
41f1acc to
7c4df44
Compare
| // experiment. | ||
| prepareScope := func(t *testing.T, scope *Scope) {} | ||
|
|
||
| if experiment, isExperimental := experimentalFuncs[funcName]; isExperimental { |
There was a problem hiding this comment.
This definitely achieves the goal of testing both cases (experimental and non-experimental), but I think got stuck in the idea that this all has to happen in one test (which I don't believe to be the case).
I found this a bit difficult to understand (multiple loops through the test cases) and might be to future readers as well, so I will share that but approve this as it does have test coverage, but I don't think "it all must be tested in one large test" is a constraint that needs to be prioritized in future work.
There was a problem hiding this comment.
This particular test has indeed grown a bit beyond its original purpose of "make sure every registered function is at least callable", but I'm not really sure how to separate out dealing with experiments because by definition they make it so that registered functions are not callable, and so the test would at least need to know enough to skip them so another test could deal with them, and then we'd have two tests that both have to be aware of which functions are experimental, which I don't think would really get the usual benefits of having separated tests (that is: them each being simple and testing one specific thing).
Indeed though, we can probably find another way to deal with this down the road.
| // be consistent with how experiment checking in the "configs" | ||
| // package itself works. The nil check here is for robustness in | ||
| // incompletely-mocked testing situations; mc should never be nil in | ||
| // real situations. |
There was a problem hiding this comment.
Excellent, thank you for adding this comment!
So far all of our language experiments have been new constructs handled statically up in the configs package, but functions are another common extention point where experiments could be useful to gather feedback and so this intends to pass the information down into the right place to allow for that to happen, even though as of this commit there are no experimental functions to use it.
This is a new part of the existing module_variable_optional_attrs experiment, because it's intended to complement the ability to declare an input variable whose type constraint is an object type with optional attributes. Module authors can use this to replace null values (that were either explicitly set or implied by attribute omission) with other non-null values of the same type. This function is a bit more type-fussy than our functions typically are because it's intended for use primarily with input variables that have fully-specified type constraints, and thus it uses that type information to help inform how the defaults data structure should be interpreted. Other uses of this function will probably be harder today because it takes a lot of extra annotation to build a value of a specific type if it isn't passing through a variable type constraint. Perhaps later language features for more general type conversion will make this more applicable, but for now the more general form of this problem is better solved other ways.
apparentlymart
force-pushed
the
f-experimental-funcs
branch
from
7c4df44 to
cec4578
Compare
|
I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues. If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further. |
ghost
locked as resolved and limited conversation to collaborators
This is a new part of the existing
module_variable_optional_attrsexperiment, because it's intended to complement the ability to declare an input variable whose type constraint is an object type with optional attributes. Module authors can use this to replace null values (that were either explicitly set or implied by attribute omission) with other non-null values of the same type.This function is a bit more type-fussy than our functions typically are because it's intended for use primarily with input variables that have fully-specified type constraints, and thus it uses that type information to help inform how the defaults data structure should be interpreted.
Other uses of this function will probably be harder today because it takes a lot of extra annotation to build a value of a specific type if it isn't passing through a variable type constraint. Perhaps later language features for more general type conversion will make this more applicable, but for now the more general form of this problem is better solved other ways.
This PR also includes a commit to allow functions to be marked as experimental. That will hopefully also prove useful to help us try out other complex functions that might need multiple design iterations in the future.