-
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 #24
Comments
Once registered, the formula should return an address so we can watch for #135 that are related to it. |
I assume that normalizing formula means returning it unparsed from AST (so, for example, with all formula names capitalized). I also assume that validation means validating syntax of the entered formula. I think that using multiple sheets (each sheet for each formula) would be better, because it will work with matrices. If we would add one temporary formula in |
We've discussed it, and maybe indeed one worksheet would be better, if we only would assume that we do not approve matrix formulas in these temporary formulas. Is that ok for you? |
* Add initial version of calculateFormula() Issue: #24 * Add test which checks whether external formula is recomputed * Add possibility to change contents of external formula * Add possibility to normalize formula * Refactoring: Introduce ChangeList type * Add test ensuring that it works for more formulas * Refactoring: Split test suite into two * Add basic validation of external formulas * Add documentation * External formula is not valid if it has lexical errors * Literal error is ok (if that's what user want) * Extract NamedReferences module * Refactoring: Rename HyperFormula#calculateFormula -> HyperFormula#addNamedExpression * Add ability to retrieve value of named expression by it's name Temporarily changing named expression formula is forbidden, need to do new API for that which does not use -1 sheet internal. * Allow to add only one expression for given name * Add assertion ensuring that NamedExpression won't contain duplicates * Add basic validation of named expressions * Refactoring: Rename NamedExpressionAlreadyTaken -> NamedExpressionNameIsAlreadyTaken * Refactoring: Extract error classes out to separate file They still belongs to the responsibility of the HyperFormula unit, but majority of this code is just formatting the error. So I think it's cleaner when they are in separate file. * Refactoring: Remove unused return value * Refactoring: Rename ivar NamedExpressions#nextExternalFormulaId -> nextNamedExpressionRow * Make it possible to retrieve value of non existing named expression * Add API for removing named expressions * Refactoring: extract NamedExpressions#buildAddress method * Refactoring: Remove magic constant * SparseStrategy will not leak memory in the long run for named expressions * Allow again to change formulas in named expressions * Refactoring * Does not allow to change not existing named expression * Add possibility to list existing named expressions * Prerefactoring: Extract class NamedExpression * Remove unnecessary bang * Add assertion in NamedExpressions#changeNamedExpressionFormula * Make named expressions names to be case insensitive * Refactoring: Extract a NamedExpressionsStore * Refactoring: Extract method NamedExpressions#storeFormulaInCell * No longer workarounds for keeping sheet = -1 * Refactoring: Use ChangeList instead of CellValueChange[] Because in a moment I want to add different kind of element to ChangeList -- the one which will tell that named expression has changed. * lint-fix * Add documentation * Fixes after merge conflict * Resolving conflicts is so. much. fun. Co-authored-by: Jakub Kowalski <kowalski_jakub@hotmail.com>
@swistak35, I know we spoke that those are basically the same as named expressions but I would keep the Also, I didn't had a chance to do a review so here are my comments: Current
We know that we will need more details about the expression: { value, scope, comment, visibility } so we should be ready for the object and avoid breaking changes in the near future. Or we can add config after the value: addNamedExpression(name, value, { scope, comment, visibility })
hyperformula/test/named-expressions.spec.ts Lines 44 to 46 in be57f95
This depends on the scope . We can have one global name but each sheet can have its own.
We can edit much more than formula. IMO name changeNamedExpressionFormula is misleading, the value can be a number, boolean or string. I hope, I don't see a test for that.
When we will have What happens when we remove a named expression? From what I see its address is lost forever. We always increment it and will never reuse a row. Can it create a problem for us when multiple named expressions are created and removed over time? Just a random thought: I would move hyperformula/src/HyperFormula.ts Lines 123 to 124 in be57f95
To a helper private method initializeNamedExpressions so someone in the future would know straight away that the -1 sheet is created for them and they have to go in pair in case of refactoring.
Do we have the name of the named expression in the changes list? or is it only |
Note to self (named expressions are not yet available within syntax so this have to wait) This is awesome 🏆 as it solves #27 and #5 at the same time. We should add a test with const engine = HyperFormula.buildFromArray([
['=TRUE()', '=TRUE', '=A1=B1'],
['=FALSE()', '=FALSE', '=A2=B2'],
['=FALSE', '=TRUE', '=A3=B3'],
])
engine.addNamedExpression('TRUE', '=True()')
engine.addNamedExpression('FALSE', '=False()')
expect(engine.getCellValue('C1')).toEqual(true)
expect(engine.getCellValue('C2')).toEqual(true)
expect(engine.getCellValue('C3')).toEqual(false) So we can confirm it and close both issues. |
As discussed,
|
For that reason I've proposed an additional argument to the API: public calculateFormula(formula: string, cellPosition: SimpleCellAddress): any; where
Yup, I agree. We should give flexibility but avoid any magic assumptions.
I think that we understood those the same. But here's my idea: |
yes, but for now only workbook level is supporter (because my understanding is that we want to do #126 in March and I don't want to constantly increase scope in #24 :) )
No, note that we specifically ask for Sparse strategy for address mapping, so it's performant with sheet which has a lot of gaps, and doesn't lose memory.
Good catch!
Actually it's the other way around. We have a name (which is unique in given scope), we (will) have a scope (and sheet, if scope is worksheet). I didn't want |
Fair point. I just want to keep other features in mind so we don't have to break the API next month.
Awesome 👍
True, we should keep |
I think that #192 exhaust that issue, there are still things to do in terms of named expression, but that one is done.
I've added |
I would prefer EDIT: Is it optional? EDIT2: The name hyperformula/src/HyperFormula.ts Line 859 in e1d91aa
|
|
Then we will break the naming pattern. ChangeNamedExpression should be sufficient? Or updateNamedExpression? |
I think that assuming that we always calculate formula from
No. There's always some context, I could make it default to "first sheet" or something like that, but isn't that more confusing? Anything else to do here? |
Description
The point is to provide public API for the methods we already have internally:
While
normalize
might be a static,validate
andcalculate
should be considered with the context of the workbook.Use case
A full use case is described in #17 but lets link to the screenshot once again:
Events
Extracted to #135
Problem?
The formula is stored outside of the graph. We won't know if it should be updated. We would have to recalculate each time something changes in the workbook.Is there any alternative? Should we register the formula in HyperFormula so it would become a part of the graph?Solved by @bardek8! Formulas will be added to
sheet
that is indexed at-1
. It will be a part of the graph and change lists.The text was updated successfully, but these errors were encountered: