Calculator.make(fn) should work for all functions, not just 0-paramater ones #2694
Conversation
* def-improvements-naming: Fixed tests Minor adjustment Show FnDocumentation with html Convert definitions to HTML Added documentation flag for namespace is optional Added named arguments for distributions Show documentation in view sidebar, when BuiltIn function is returned Show tuples with [] Cleaned up mixture to use optional Adds names to many types In documentation, changed distribution -> dist
🦋 Changeset detectedLatest commit: 89e330f The changes in this PR will be included in the next version bump. This PR includes changesets to release 4 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
1 Ignored Deployment
|
Apply Sweep Rules to your PR?
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2694 +/- ##
==========================================
- Coverage 71.54% 71.30% -0.24%
==========================================
Files 118 118
Lines 6474 6497 +23
Branches 1327 1339 +12
==========================================
+ Hits 4632 4633 +1
- Misses 1834 1856 +22
Partials 8 8 ☔ View full report in Codecov by Sentry. |
OAGr
changed the title
| @@ -153,6 +160,13 @@ export class SqLambdaValue extends SqAbstractValue<"Lambda", SqLambda> { | |||
| asJS() { | |||
| return this.value; // SqLambda is nicer than internal Lambda, so we use that | |||
| } | |||
|
|
|||
| toCalculator(): SqCalculatorValue | undefined { | |||
There was a problem hiding this comment.
I used this for testing, but we're not using it otherwise right now. But it seems good to have.
There was a problem hiding this comment.
Happy to see this feature implemented.
I'm hesitant about frTypes.ts and lambda.ts containing fields and methods that are needed for calculators, it seems like a violation of abstraction layers. I'd be more comfortable with frTypeToInput doing that job.
But I understand that it's a bit hard to do now because FRType doesn't always expose enough information yet. So it's not a problem if we merge this now and then refactor it later when FRType is converted to classes (which I plan to do soon).
| keepBoxes?: boolean; | ||
| isOptional?: boolean; | ||
| tag?: string; | ||
| underlyingType?: FRType<any>; |
There was a problem hiding this comment.
This is not used anywhere.
Before, Calculator.make(fn), without passed-in columns, would only work if the function had zero params.
With this, it works more generally.
Most of the work was getting it to work with defaults / correct types, for FrTypes. This only helps for built-in functions, but could be useful for building on top of later on.
I kind of would like to wait for this, before doing testing:
#2739