-
Notifications
You must be signed in to change notification settings - Fork 108
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
Formula helpers and named expressions (part 2) #192
Conversation
test/named-expressions.spec.ts
Outdated
@@ -54,6 +56,14 @@ describe('Named expressions', () => { | |||
}).toThrowError("Name of Named Expression '1definitelyIncorrectName' is invalid") | |||
}) | |||
|
|||
it('when adding named expression, only formulas are accepted', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this limitation necessary? Named expressions should accept any expression, not only formulas. Values, other named expressions, constant arrays, ranges should be supported as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note: normalizeFormula
, validateFormula
, calculateFormula
as indicated by their names should support only formulas, those should not change. My comment is for named expressions only
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought that we want to focus on formulas for now, we can surely change that to accept values too
return this.address.sheet | ||
} | ||
|
||
public get value() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should ExportedNamedExpressionChange
have value
alias as well?
Or we don't need newValue
when we have value
? It's a little bit confusing why we have both while they are always equal.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@wojciechczerniak only for compatibility reasons (I didn't want to break the integration), I'd leave only newValue as it betters conveys intent, but the value is already there, if our current integration doesn't rely on that then I will gladly fallback to newValue only
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jansiegel has to review the integration anyway. A lot has changed since we did it, and since we haven't released HF or HOT integration yet so we should break the compatibility if it's necessary. Better now than later or keeping it. Maintaining the unnecessary code is a chore.
Because in a moment I will introduce second class, representing changed named expression.
c4a1ee1
to
3f9301e
Compare
Since formula is not stored, temporary sounds better
not directly know that named expression is stored in some specific cell.
Context
This changes a little bit the output of
changes
when doing CRUD operations, but the class which is returned now is compatible (in terms of API) with previously returned object.How has this been tested?
Tests
Related issue(s):
Checklist: