Pu/mergeall #667
Pu/mergeall #667
Conversation
|
This pull request introduces 12 alerts and fixes 4 when merging 6a07bf1 into cf800ce - view on LGTM.com new alerts:
fixed alerts:
|
Codecov Report
@@ Coverage Diff @@
## develop #667 +/- ##
========================================
Coverage 92.43% 92.44%
========================================
Files 167 167
Lines 39090 39102 +12
Branches 3563 3565 +2
========================================
+ Hits 36134 36146 +12
Misses 2950 2950
Partials 6 6
|
| @@ -6,6 +6,7 @@ | |||
| import {SimpleCellAddress} from '../../Cell' | |||
| import {RawCellContent} from '../../CellContentParser' | |||
| import {EmptyValue, InterpreterValue} from '../../interpreter/InterpreterValue' | |||
| import {Maybe} from '../../Maybe' | |||
There was a problem hiding this comment.
Added line is not used.
BTW EmptyCellVertex is also imported but never used can be removed as well
There was a problem hiding this comment.
Or should the getCell type be updated?
- getCell(address: SimpleCellAddress): CellVertex | null
+ getCell(address: SimpleCellAddress): Maybe<CellVertex>There was a problem hiding this comment.
Let us do the cleanup of imports: once, automatically, on the develop, once all the branches are merged. Webstorm can do this, but this will touch basically all the files. If we do it here, it will be hell to merge with follow-up branches.
There was a problem hiding this comment.
Or should the
getCelltype be updated?- getCell(address: SimpleCellAddress): CellVertex | null + getCell(address: SimpleCellAddress): Maybe<CellVertex>
at one point yes. This is on my internal todo-list.
test/engine.spec.ts
Outdated
| expect(engine.getCellFormula(adr('A4'))).toEqual(undefined) | ||
| expect(engine.getCellFormula(adr('B3'))).toEqual(undefined) | ||
| expect(engine.getCellFormula(adr('B4'))).toEqual(undefined) |
There was a problem hiding this comment.
Does it make sense to check those cells? Is the description correct, matrix not array?
There was a problem hiding this comment.
we'll do big unification of naming, also on internal todo list
test/engine.spec.ts
Outdated
| ]) | ||
|
|
||
| expect(engine.getCellValue(adr('A1'))).toEqualError(detailedError(ErrorType.ERROR, ErrorMessage.ParseError)) | ||
| expect(engine.getCellFormula(adr('A1'))).toEqual('{=TRANSPOSE(}') | ||
| expect(engine.getCellFormula(adr('A1'))).toEqual('=TRANSPOSE(') |
There was a problem hiding this comment.
IMO this tests is a duplicate of parse error above? Is the description correct with matrix and not array?
There was a problem hiding this comment.
afair fixed in the followup branch
|
This pull request introduces 1 alert and fixes 1 when merging 4c2851a into e624cc3 - view on LGTM.com new alerts:
fixed alerts:
|
|
This pull request introduces 1 alert and fixes 1 when merging 7e3d1d5 into e624cc3 - view on LGTM.com new alerts:
fixed alerts:
|
Context
Merge of PR #628 #629 #630 #652
How has this been tested?
Types of changes
Related issue(s):
Checklist: