Refactor SheetMapping

Author: sequba

src/DependencyGraph/DependencyGraph.ts

@@ -322,7 +322,7 @@ export class DependencyGraph {
       this.sheetMapping.removeSheet(sheetBeingRemoved)
       this.addressMapping.removeSheet(sheetBeingRemoved)
     } else {
-      this.sheetMapping.softRemoveSheet(sheetBeingRemoved)
+      this.sheetMapping.markSheetAsPlaceholder(sheetBeingRemoved)
     }
 
     this.stats.measure(StatType.ADJUSTING_RANGES, () => {
@@ -624,23 +624,23 @@ export class DependencyGraph {
   }
 
   public getCellValue(address: SimpleCellAddress): InterpreterValue {
-    if (address.sheet !== NamedExpressions.SHEET_FOR_WORKBOOK_EXPRESSIONS && !this.sheetMapping.hasSheetWithId(address.sheet, { includeNotAdded: false })) {
+    if (address.sheet !== NamedExpressions.SHEET_FOR_WORKBOOK_EXPRESSIONS && !this.sheetMapping.hasSheetWithId(address.sheet, { includePlaceholders: false })) {
       return new CellError(ErrorType.REF, ErrorMessage.SheetRef)
     }
 
     return this.addressMapping.getCellValue(address)
   }
 
   public getRawValue(address: SimpleCellAddress): RawCellContent {
-    if (address.sheet !== NamedExpressions.SHEET_FOR_WORKBOOK_EXPRESSIONS && !this.sheetMapping.hasSheetWithId(address.sheet, { includeNotAdded: false })) {
+    if (address.sheet !== NamedExpressions.SHEET_FOR_WORKBOOK_EXPRESSIONS && !this.sheetMapping.hasSheetWithId(address.sheet, { includePlaceholders: false })) {
       return null
     }
 
     return this.addressMapping.getRawValue(address)
   }
 
   public getScalarValue(address: SimpleCellAddress): InternalScalarValue {
-    if (address.sheet !== NamedExpressions.SHEET_FOR_WORKBOOK_EXPRESSIONS && !this.sheetMapping.hasSheetWithId(address.sheet, { includeNotAdded: false })) {
+    if (address.sheet !== NamedExpressions.SHEET_FOR_WORKBOOK_EXPRESSIONS && !this.sheetMapping.hasSheetWithId(address.sheet, { includePlaceholders: false })) {
       return new CellError(ErrorType.REF, ErrorMessage.SheetRef)
     }
 
@@ -1196,11 +1196,11 @@ export class DependencyGraph {
 
     const hasDependentInExistingSheet = dependents.some(addr => {
       if (isSimpleCellAddress(addr)) {
-        return addr.sheet === NamedExpressions.SHEET_FOR_WORKBOOK_EXPRESSIONS || this.sheetMapping.hasSheetWithId(addr.sheet, { includeNotAdded: false })
+        return addr.sheet === NamedExpressions.SHEET_FOR_WORKBOOK_EXPRESSIONS || this.sheetMapping.hasSheetWithId(addr.sheet, { includePlaceholders: false })
       }
 
       if (isSimpleCellRange(addr)) {
-        return this.sheetMapping.hasSheetWithId(addr.start.sheet, { includeNotAdded: false }) || this.sheetMapping.hasSheetWithId(addr.end.sheet, { includeNotAdded: false })
+        return this.sheetMapping.hasSheetWithId(addr.start.sheet, { includePlaceholders: false }) || this.sheetMapping.hasSheetWithId(addr.end.sheet, { includePlaceholders: false })
       }
 
       return false

src/DependencyGraph/SheetMapping.ts

@@ -11,7 +11,7 @@ import {Maybe} from '../Maybe'
  * Options for querying the sheet mapping.
  */
 export interface SheetMappingQueryOptions {
-  includeNotAdded?: boolean,
+  includePlaceholders?: boolean,
 }
 
 /**
@@ -21,12 +21,8 @@ class Sheet {
   constructor(
     public readonly id: number,
     public displayName: string,
-    /**
-     * Whether the sheet has been explicitly added to the instance either on initialization or via addSheet method.
-     */
-    public isAdded: boolean = true,
-  ) {
-  }
+    public isPlaceholder: boolean = false,
+  ) {}
 
   /**
    * Returns the canonical (normalized) name of the sheet.
@@ -39,9 +35,9 @@ class Sheet {
 /**
  * Manages the sheets in the instance.
  * Can convert between sheet names and ids and vice versa.
- * Also stores placeholders for sheets that are used in formulas but not yet added. They are marked as isAdded=false.
+ * Also stores placeholders for sheets that are used in formulas but not yet added. They are marked as isPlaceholder=true.
  * Sheetnames thet differ only in case are considered the same. (See: canonicalizeSheetName)
-*/
+ */
 export class SheetMapping {
   /**
    * Prefix for new sheet names if no name is provided by the user
@@ -73,16 +69,14 @@ export class SheetMapping {
   }
 
   /**
-   * Returns sheet ID for the given name (case-insensitive). By default excludes not added sheets.
-   *
-   * @returns {Maybe<number>} the sheet ID, or undefined if not found.
+   * Returns sheet ID for the given name. By default excludes placeholders.
    */
   public getSheetId(sheetName: string, options: SheetMappingQueryOptions = {}): Maybe<number> {
     return this._getSheetByName(sheetName, options)?.id
   }
 
   /**
-   * Returns sheet ID for the given name. Excludes not added sheets.
+   * Returns sheet ID for the given name. Excludes placeholders.
    *
    * @throws {NoSheetWithNameError} if the sheet with the given name does not exist.
    */
@@ -96,7 +90,7 @@ export class SheetMapping {
   }
 
   /**
-   * Returns display name for the given sheet ID. Excludes not added sheets.
+   * Returns display name for the given sheet ID. Excludes placeholders.
    *
    * @returns {Maybe<string>} the display name, or undefined if the sheet with the given ID does not exist.
    */
@@ -105,7 +99,7 @@ export class SheetMapping {
   }
 
   /**
-   * Returns display name for the given sheet ID. Excludes not added sheets.
+   * Returns display name for the given sheet ID. Excludes placeholders.
    *
    * @throws {NoSheetWithIdError} if the sheet with the given ID does not exist.
    */
@@ -114,60 +108,59 @@ export class SheetMapping {
   }
 
   /**
-   * Iterates over all sheet display names. By default excludes not added sheets.
+   * Iterates over all sheet display names. By default excludes placeholders.
    */
   public* iterateSheetNames(options: SheetMappingQueryOptions = {}): IterableIterator<string> {
     for (const sheet of this.allSheets.values()) {
-      if (options.includeNotAdded || sheet.isAdded) {
+      if (options.includePlaceholders || !sheet.isPlaceholder) {
         yield sheet.displayName
       }
     }
   }
 
   /**
-   * Returns array of all sheet display names. By default excludes not added sheets.
+   * Returns array of all sheet display names. By default excludes placeholders.
    */
   public getSheetNames(options: SheetMappingQueryOptions = {}): string[] {
     return Array.from(this.iterateSheetNames(options))
   }
 
   /**
-   * Returns total count of sheets. By default excludes not added sheets.
+   * Returns total count of sheets. By default excludes placeholders.
    */
   public numberOfSheets(options: SheetMappingQueryOptions = {}): number {
     return this.getSheetNames(options).length
   }
 
   /**
-   * Checks if sheet with given ID exists. By default excludes not added sheets.
+   * Checks if sheet with given ID exists. By default excludes placeholders.
    */
   public hasSheetWithId(sheetId: number, options: SheetMappingQueryOptions = {}): boolean {
     return this._getSheet(sheetId, options) !== undefined
   }
 
   /**
-   * Checks if sheet with given name exists (case-insensitive). Excludes not added sheets.
+   * Checks if sheet with given name exists (case-insensitive). Excludes placeholders.
    */
   public hasSheetWithName(sheetName: string): boolean {
     return this._getSheetByName(sheetName, {}) !== undefined
   }
 
   /**
    * Adds new sheet with optional name and returns its ID.
-   * If called with a reserved sheet name (sheet name of some sheet present in the mapping but not added yet), adds the reserved sheet.
+   * If called with a name of placeholder sheet, adds the real sheet.
    *
    * @throws {SheetNameAlreadyTakenError} if the sheet with the given name already exists.
-   * @returns {number} the ID of the new sheet.
    */
   public addSheet(newSheetDisplayName: string = `${this.sheetNamePrefix}${this.lastSheetId + 2}`): number {
-    const sheetWithConflictingName = this._getSheetByName(newSheetDisplayName, { includeNotAdded: true })
+    const sheetWithConflictingName = this._getSheetByName(newSheetDisplayName, { includePlaceholders: true })
 
     if (sheetWithConflictingName) {
-      if (sheetWithConflictingName.isAdded) {
+      if (!sheetWithConflictingName.isPlaceholder) {
         throw new SheetNameAlreadyTakenError(newSheetDisplayName)
       }
 
-      sheetWithConflictingName.isAdded = true
+      sheetWithConflictingName.isPlaceholder = false
       return sheetWithConflictingName.id
     }
 
@@ -178,49 +171,48 @@ export class SheetMapping {
   }
 
   /**
-   * Stores the sheet in the mapping with flag isAdded=false.
-   * If such sheet name is already present in the mapping, does nothing.
-   *
-   * @returns {number} the ID of the reserved sheet.
+   * Adds a placeholder sheet with the given name if it does not exist yet
    */
-  public reserveSheetName(sheetName: string): number {
-    const sheetWithConflictingName = this._getSheetByName(sheetName, { includeNotAdded: true })
+  public addPlaceholderIfNotExists(sheetName: string): number {
+    const sheetWithConflictingName = this._getSheetByName(sheetName, { includePlaceholders: true })
 
     if (sheetWithConflictingName) {
       return sheetWithConflictingName.id
     }
 
     this.lastSheetId++
-    const sheet = new Sheet(this.lastSheetId, sheetName, false)
+    const sheet = new Sheet(this.lastSheetId, sheetName, true)
     this.storeSheetInMappings(sheet)
     return sheet.id
   }
 
   /**
-   * Removes sheet with given ID. Ignores not added sheets.
-   *
-   * @throws {NoSheetWithIdError} if the sheet with the given ID does not exist or is not added yet.
+   * Removes sheet with given ID. Ignores placeholders
+   * @throws {NoSheetWithIdError} if the sheet with the given ID does not exist or is a placeholder
    */
   public removeSheet(sheetId: number): void {
-    const sheet = this._getSheetOrThrowError(sheetId, {})
+    const sheet = this._getSheetOrThrowError(sheetId, { includePlaceholders: false })
     this.allSheets.delete(sheetId)
     this.mappingFromCanonicalNameToId.delete(sheet.canonicalName)
   }
 
-  public softRemoveSheet(sheetId: number): void {
+  /**
+   * Marks sheet with given ID as a placeholder.
+   * @throws {NoSheetWithIdError} if the sheet with the given ID does not exist
+   */
+  public markSheetAsPlaceholder(sheetId: number): void {
     const sheet = this._getSheetOrThrowError(sheetId, {})
-    sheet.isAdded = false
+    sheet.isPlaceholder = true
   }
 
   /**
    * Renames sheet.
-   * If called with sheetId of a not added sheet, throws {NoSheetWithIdError}.
+   * If called with sheetId of a placeholder sheet, throws {NoSheetWithIdError}.
    * If newDisplayName is conflicting with an existing sheet, throws {SheetNameAlreadyTakenError}.
-   * If newDisplayName is conflicting with a reserved sheet name (name of a non-added sheet), throws {SheetNameAlreadyTakenError}.
+   * If newDisplayName is conflicting with a placeholder sheet name, throws {SheetNameAlreadyTakenError}.
    *
    * @throws {SheetNameAlreadyTakenError} if the sheet with the given name already exists.
    * @throws {NoSheetWithIdError} if the sheet with the given ID does not exist.
-   * @returns {Maybe<string>} the old name, or undefined if the name was not changed.
    */
   public renameSheet(sheetId: number, newDisplayName: string): { previousDisplayName: Maybe<string>, mergedSheetWith?: number } {
     const sheet = this._getSheetOrThrowError(sheetId, {})
@@ -230,11 +222,11 @@ export class SheetMapping {
       return { previousDisplayName: undefined }
     }
 
+    const sheetWithConflictingName = this._getSheetByName(newDisplayName, { includePlaceholders: true })
     let mergedSheetWith: number | undefined = undefined
-    const sheetWithConflictingName = this._getSheetByName(newDisplayName, { includeNotAdded: true })
 
     if (sheetWithConflictingName !== undefined && sheetWithConflictingName.id !== sheet.id) {
-      if (sheetWithConflictingName.isAdded) {
+      if (!sheetWithConflictingName.isPlaceholder) {
         throw new SheetNameAlreadyTakenError(newDisplayName)
       } else {
         this.mappingFromCanonicalNameToId.delete(sheetWithConflictingName.canonicalName)
@@ -277,7 +269,7 @@ export class SheetMapping {
       return undefined
     }
 
-    return (options.includeNotAdded || retrievedSheet.isAdded) ? retrievedSheet : undefined
+    return (options.includePlaceholders || !retrievedSheet.isPlaceholder) ? retrievedSheet : undefined
   }
 
   /**

src/DependencyGraph/SheetReferenceRegistrar.ts

@@ -23,7 +23,7 @@ export class SheetReferenceRegistrar {
    * @returns the sheet identifier that represents the referenced sheet
    */
   public ensureSheetRegistered(sheetName: string): number {
-    const sheetId = this.sheetMapping.reserveSheetName(sheetName)
+    const sheetId = this.sheetMapping.addPlaceholderIfNotExists(sheetName)
     this.addressMapping.addSheetStrategyPlaceholderIfNotExists(sheetId)
     return sheetId
   }

src/interpreter/Interpreter.ts

@@ -288,7 +288,7 @@ export class Interpreter {
   }
 
   private isSheetJustReserved(address: AddressWithSheet): boolean {
-    return address.sheet !== undefined && address.sheet !== NamedExpressions.SHEET_FOR_WORKBOOK_EXPRESSIONS && !this.dependencyGraph.sheetMapping.hasSheetWithId(address.sheet, { includeNotAdded: false })
+    return address.sheet !== undefined && address.sheet !== NamedExpressions.SHEET_FOR_WORKBOOK_EXPRESSIONS && !this.dependencyGraph.sheetMapping.hasSheetWithId(address.sheet, { includePlaceholders: false })
   }
 
   private rangeSpansOneSheet(ast: CellRangeAst | ColumnRangeAst | RowRangeAst): boolean {

src/interpreter/plugin/NumericAggregationPlugin.ts

@@ -629,7 +629,7 @@ export class NumericAggregationPlugin extends FunctionPlugin implements Function
       }
     }
 
-    if (!this.dependencyGraph.sheetMapping.hasSheetWithId(range.start.sheet, { includeNotAdded: false }) || !this.dependencyGraph.sheetMapping.hasSheetWithId(range.end.sheet, { includeNotAdded: false })) {
+    if (!this.dependencyGraph.sheetMapping.hasSheetWithId(range.start.sheet, { includePlaceholders: false }) || !this.dependencyGraph.sheetMapping.hasSheetWithId(range.end.sheet, { includePlaceholders: false })) {
       return new CellError(ErrorType.REF, ErrorMessage.SheetRef)
     }
 

src/parser/Unparser.ts

@@ -109,7 +109,7 @@ export class Unparser {
   }
 
   private unparseSheetName(sheetId: number): string {
-    const sheetName = sheetIndexToString(sheetId, id => this.sheetMapping.getSheetNameOrThrowError(id, { includeNotAdded: true }))
+    const sheetName = sheetIndexToString(sheetId, id => this.sheetMapping.getSheetNameOrThrowError(id, { includePlaceholders: true }))
     if (sheetName === undefined) {
       throw new NoSheetWithIdError(sheetId)
     }

test/unit/dependency-graph-sheet-reference-registrar.spec.ts

@@ -13,7 +13,7 @@ describe('SheetReferenceRegistrar', () => {
     const secondSheetId = registrar.ensureSheetRegistered('Ghost')
 
     expect(firstSheetId).toBe(secondSheetId)
-    expect(sheetMapping.getSheetId('Ghost', {includeNotAdded: true})).toBe(firstSheetId)
+    expect(sheetMapping.getSheetId('Ghost', {includePlaceholders: true})).toBe(firstSheetId)
     expect(() => addressMapping.getStrategyForSheet(firstSheetId)).not.toThrow()
   })
 })

test/unit/graphComparator.ts

@@ -21,8 +21,8 @@ export class EngineComparator {
   }
 
   public compare(): void {
-    const expectedNumberOfSheets = this.expected.sheetMapping.numberOfSheets({includeNotAdded: true})
-    const numberOfSheets = this.actual.sheetMapping.numberOfSheets({includeNotAdded: true})
+    const expectedNumberOfSheets = this.expected.sheetMapping.numberOfSheets({includePlaceholders: true})
+    const numberOfSheets = this.actual.sheetMapping.numberOfSheets({includePlaceholders: true})
 
     if (expectedNumberOfSheets !== numberOfSheets) {
       throw Error(`Expected number of sheets ${expectedNumberOfSheets}, actual: ${numberOfSheets}`)