Merge pull request #144 from hed-standard/add-tsv-line-to-issues

Add TSV line numbers to TSV-related issue messages
hed-standard · Mar 29, 2024 · 2d3cc74 · 2d3cc74
2 parents 6961b2c + 7d149dc
commit 2d3cc74
Show file tree

Hide file tree

Showing 7 changed files with 117 additions and 45 deletions.
diff --git a/bids/types/issues.js b/bids/types/issues.js
@@ -27,12 +27,23 @@ export class BidsIssue {
 
   /**
    * Whether this issue is an error.
+   *
    * @returns {boolean}
    */
   isError() {
     return bidsHedErrorCodes.has(this.code)
   }
 
+  /**
+   * Determine if any of the passed issues are errors.
+   *
+   * @param {BidsIssue[]} issues A list of issues.
+   * @return {boolean} Whether any of the passed issues are errors (rather than warnings).
+   */
+  static anyAreErrors(issues) {
+    return issues.some((issue) => issue.isError())
+  }
+
   static generateInternalErrorPromise(error) {
     return Promise.resolve([new BidsIssue(106, null, error.message)])
   }

diff --git a/bids/validate.js b/bids/validate.js
@@ -88,6 +88,6 @@ class BidsHedValidator {
    */
   _pushIssues(issues) {
     this.issues.push(...issues)
-    return issues.some((issue) => issue.isError())
+    return BidsIssue.anyAreErrors(issues)
   }
 }
diff --git a/bids/validator/bidsHedColumnValidator.js b/bids/validator/bidsHedColumnValidator.js
@@ -55,8 +55,8 @@ export class BidsHedColumnValidator {
    * @private
    */
   _validateFileHedColumn(eventFileData) {
-    return eventFileData.hedColumnHedStrings.flatMap((hedString) =>
-      this._validateFileHedColumnString(eventFileData, hedString),
+    return eventFileData.hedColumnHedStrings.flatMap((hedString, rowIndexMinusTwo) =>
+      this._validateFileHedColumnString(eventFileData, hedString, rowIndexMinusTwo + 2),
     )
   }
 
@@ -65,10 +65,11 @@ export class BidsHedColumnValidator {
    *
    * @param {BidsEventFile} eventFileData The BIDS event file whose HED column is to be validated.
    * @param {string} hedString The string to be validated.
+   * @param {number} rowIndex The index of this row in the TSV file.
    * @returns {BidsHedIssue[]} All issues found.
    * @private
    */
-  _validateFileHedColumnString(eventFileData, hedString) {
+  _validateFileHedColumnString(eventFileData, hedString, rowIndex) {
     if (!hedString) {
       return []
     }
@@ -81,7 +82,9 @@ export class BidsHedColumnValidator {
     }
 
     const [parsedString, parsingIssues] = parseHedString(hedString, this.hedSchemas)
-    issues.push(...BidsHedIssue.fromHedIssues(Object.values(parsingIssues).flat(), eventFileData.file))
+    issues.push(
+      ...BidsHedIssue.fromHedIssues(Object.values(parsingIssues).flat(), eventFileData.file, { tsvLine: rowIndex }),
+    )
 
     if (parsedString === null) {
       return issues
@@ -90,15 +93,18 @@ export class BidsHedColumnValidator {
     if (parsedString.columnSplices.length > 0) {
       issues.push(
         BidsHedIssue.fromHedIssue(
-          generateIssue('curlyBracesInHedColumn', { column: parsedString.columnSplices[0].format() }),
+          generateIssue('curlyBracesInHedColumn', {
+            column: parsedString.columnSplices[0].format(),
+            tsvLine: rowIndex,
+          }),
           eventFileData.file,
         ),
       )
       return issues
     }
 
     const [, hedIssues] = validateHedString(parsedString, this.hedSchemas, options)
-    const convertedIssues = BidsHedIssue.fromHedIssues(hedIssues, eventFileData.file)
+    const convertedIssues = BidsHedIssue.fromHedIssues(hedIssues, eventFileData.file, { tsvLine: rowIndex })
     issues.push(...convertedIssues)
 
     return issues

diff --git a/bids/validator/bidsHedTsvValidator.js b/bids/validator/bidsHedTsvValidator.js
@@ -1,5 +1,5 @@
 import { BidsHedSidecarValidator } from './bidsHedSidecarValidator'
-import { BidsHedIssue } from '../types/issues'
+import { BidsHedIssue, BidsIssue } from '../types/issues'
 import { BidsTsvEvent, BidsTsvRow } from '../types/tsv'
 import { parseHedString } from '../../parser/main'
 import ColumnSplicer from '../../parser/columnSplicer'
@@ -57,20 +57,70 @@ export class BidsHedTsvValidator {
       return this.issues
     }
 
-    const hedStrings = this.parseHed()
-    if (hedStrings.length > 0) {
+    const bidsHedTsvParser = new BidsHedTsvParser(this.tsvFile, this.hedSchemas)
+    const hedStrings = bidsHedTsvParser.parse()
+    this.issues.push(...bidsHedTsvParser.issues)
+    if (!BidsIssue.anyAreErrors(this.issues)) {
       this.validateCombinedDataset(hedStrings)
     }
 
     return this.issues
   }
 
+  /**
+   * Validate the HED data in a combined event TSV file/sidecar BIDS data collection.
+   *
+   * @param {ParsedHedString[]} hedStrings The HED strings in the data collection.
+   */
+  validateCombinedDataset(hedStrings) {
+    const [, hedIssues] = validateHedDatasetWithContext(
+      hedStrings,
+      this.tsvFile.mergedSidecar.hedStrings,
+      this.hedSchemas,
+      {
+        checkForWarnings: true,
+        validateDatasetLevel: this.tsvFile.isTimelineFile,
+      },
+    )
+    this.issues.push(...BidsHedIssue.fromHedIssues(hedIssues, this.tsvFile.file))
+  }
+}
+
+export class BidsHedTsvParser {
+  /**
+   * The BIDS TSV file being parsed.
+   * @type {BidsTsvFile}
+   */
+  tsvFile
+  /**
+   * The HED schema collection being parsed against.
+   * @type {Schemas}
+   */
+  hedSchemas
+  /**
+   * The issues found during parsing.
+   * @type {BidsHedIssue[]}
+   */
+  issues
+
+  /**
+   * Constructor.
+   *
+   * @param {BidsTsvFile} tsvFile The BIDS TSV file being parsed.
+   * @param {Schemas} hedSchemas The HED schema collection being parsed against.
+   */
+  constructor(tsvFile, hedSchemas) {
+    this.tsvFile = tsvFile
+    this.hedSchemas = hedSchemas
+    this.issues = []
+  }
+
   /**
    * Combine the BIDS sidecar HED data into a BIDS TSV file's HED data.
    *
    * @returns {ParsedHedString[]} The combined HED string collection for this BIDS TSV file.
    */
-  parseHed() {
+  parse() {
     const tsvHedRows = this._generateHedRows()
     const hedStrings = this._parseHedRows(tsvHedRows)
 
@@ -185,10 +235,9 @@ export class BidsHedTsvValidator {
     const splicedParsedString = columnSplicer.splice()
     const splicingIssues = columnSplicer.issues
     if (splicingIssues.length > 0) {
-      this.issues.push(...BidsHedIssue.fromHedIssues(splicingIssues, this.tsvFile.file))
+      this.issues.push(...BidsHedIssue.fromHedIssues(splicingIssues, this.tsvFile.file, { tsvLine }))
       return null
     }
-    splicedParsedString.context.set('tsvLine', tsvLine)
 
     return new BidsTsvRow(splicedParsedString, rowCells, this.tsvFile, tsvLine)
   }
@@ -262,22 +311,4 @@ export class BidsHedTsvValidator {
     )
     return null
   }
-
-  /**
-   * Validate the HED data in a combined event TSV file/sidecar BIDS data collection.
-   *
-   * @param {ParsedHedString[]} hedStrings The HED strings in the data collection.
-   */
-  validateCombinedDataset(hedStrings) {
-    const [, hedIssues] = validateHedDatasetWithContext(
-      hedStrings,
-      this.tsvFile.mergedSidecar.hedStrings,
-      this.hedSchemas,
-      {
-        checkForWarnings: true,
-        validateDatasetLevel: this.tsvFile.isTimelineFile,
-      },
-    )
-    this.issues.push(...BidsHedIssue.fromHedIssues(hedIssues, this.tsvFile.file))
-  }
 }
diff --git a/common/issues/issues.js b/common/issues/issues.js
@@ -76,8 +76,7 @@ export class Issue {
     this.code = internalCode
     this.hedCode = hedCode
     this.level = level
-    // Pre-convert all parameters except the substring bounds (an integer array) to their string forms.
-    this.parameters = mapValues(parameters, (value, key) => (key === 'bounds' ? value : String(value)))
+    this.parameters = parameters
     this.generateMessage()
   }
 
@@ -94,9 +93,14 @@ export class Issue {
    * (Re-)generate the issue message.
    */
   generateMessage() {
+    // Convert all parameters except the substring bounds (an integer array) to their string forms.
+    this.parameters = mapValues(this.parameters, (value, key) => (key === 'bounds' ? value : String(value)))
+
     const bounds = this.parameters.bounds ?? []
     const messageTemplate = issueData[this.internalCode].message
     let message = messageTemplate(...bounds, this.parameters)
+
+    // Special parameters
     if (this.parameters.sidecarKey) {
       message += ` Sidecar key: "${this.parameters.sidecarKey}".`
     }
@@ -106,8 +110,11 @@ export class Issue {
     if (this.parameters.hedString) {
       message += ` HED string: "${this.parameters.hedString}".`
     }
+
+    // Append link to error code in HED spec.
     const hedCodeAnchor = this.hedCode.toLowerCase().replace(/_/g, '-')
     const hedSpecLink = `For more information on this HED ${this.level}, see https://hed-specification.readthedocs.io/en/latest/Appendix_B.html#${hedCodeAnchor}`
+
     this.message = `${this.level.toUpperCase()}: [${this.hedCode}] ${message} (${hedSpecLink}.)`
   }
 

diff --git a/tests/bids.spec.js b/tests/bids.spec.js
@@ -1,13 +1,15 @@
 import chai from 'chai'
 const assert = chai.assert
+import cloneDeep from 'lodash/cloneDeep'
+
 import converterGenerateIssue from '../converter/issues'
 import { generateIssue } from '../common/issues/issues'
 import { SchemaSpec, SchemasSpec } from '../common/schema/types'
 import { buildBidsSchemas, parseSchemasSpec } from '../bids/schema'
 import { BidsDataset, BidsHedIssue, BidsIssue, validateBidsDataset } from '../bids'
 import { bidsDatasetDescriptions, bidsSidecars, bidsTsvFiles } from './bids.spec.data'
 import { parseHedString } from '../parser/main'
-import { BidsHedTsvValidator } from '../bids/validator/bidsHedTsvValidator'
+import { BidsHedTsvParser } from '../bids/validator/bidsHedTsvValidator'
 
 describe('BIDS datasets', () => {
   /**
@@ -164,14 +166,14 @@ describe('BIDS datasets', () => {
       const expectedIssues = {
         all_good: [],
         all_bad: [
-          BidsHedIssue.fromHedIssue(speedIssue, badDatasets[0].file),
-          BidsHedIssue.fromHedIssue(maglevWarning, badDatasets[1].file),
-          BidsHedIssue.fromHedIssue(speedIssue, badDatasets[2].file),
-          BidsHedIssue.fromHedIssue(speedIssue, badDatasets[3].file),
-          BidsHedIssue.fromHedIssue(maglevError, badDatasets[3].file),
-          BidsHedIssue.fromHedIssue(converterMaglevError, badDatasets[3].file),
-          BidsHedIssue.fromHedIssue(speedIssue, badDatasets[4].file),
-          BidsHedIssue.fromHedIssue(maglevWarning, badDatasets[4].file),
+          BidsHedIssue.fromHedIssue(cloneDeep(speedIssue), badDatasets[0].file, { tsvLine: 2 }),
+          BidsHedIssue.fromHedIssue(cloneDeep(maglevWarning), badDatasets[1].file, { tsvLine: 2 }),
+          BidsHedIssue.fromHedIssue(cloneDeep(speedIssue), badDatasets[2].file, { tsvLine: 3 }),
+          BidsHedIssue.fromHedIssue(converterMaglevError, badDatasets[3].file, { tsvLine: 2 }),
+          BidsHedIssue.fromHedIssue(cloneDeep(maglevError), badDatasets[3].file, { tsvLine: 2 }),
+          BidsHedIssue.fromHedIssue(cloneDeep(speedIssue), badDatasets[3].file, { tsvLine: 3 }),
+          BidsHedIssue.fromHedIssue(cloneDeep(maglevWarning), badDatasets[4].file, { tsvLine: 2 }),
+          BidsHedIssue.fromHedIssue(cloneDeep(speedIssue), badDatasets[4].file, { tsvLine: 3 }),
         ],
       }
       return validator(testDatasets, expectedIssues, specs)
@@ -202,30 +204,35 @@ describe('BIDS datasets', () => {
               tag: 'Boat',
             }),
             badDatasets[2].file,
+            { tsvLine: 5 },
           ),
           BidsHedIssue.fromHedIssue(
             generateIssue('duplicateTag', {
               tag: 'Boat',
             }),
             badDatasets[2].file,
+            { tsvLine: 5 },
           ),
           BidsHedIssue.fromHedIssue(
             generateIssue('invalidValue', {
               tag: 'Duration/ferry s',
             }),
             badDatasets[3].file,
+            { tsvLine: 2 },
           ),
           BidsHedIssue.fromHedIssue(
             generateIssue('duplicateTag', {
               tag: 'Age/30',
             }),
             badDatasets[3].file,
+            { tsvLine: 2 },
           ),
           BidsHedIssue.fromHedIssue(
             generateIssue('duplicateTag', {
               tag: 'Age/30',
             }),
             badDatasets[3].file,
+            { tsvLine: 2 },
           ),
           BidsHedIssue.fromHedIssue(
             generateIssue('sidecarKeyMissing', {
@@ -482,7 +489,7 @@ describe('BIDS datasets', () => {
       const expectedIssues = {
         bad_tsv: [
           BidsHedIssue.fromHedIssue(
-            generateIssue('illegalDefinitionContext', { string: '(Definition/myDef, (Label/Red, Green))' }),
+            generateIssue('illegalDefinitionContext', { string: '(Definition/myDef, (Label/Red, Green))', tsvLine: 2 }),
             badTsvDatasets[0].file,
           ),
         ],
@@ -542,6 +549,7 @@ describe('BIDS datasets', () => {
           BidsHedIssue.fromHedIssue(
             generateIssue('curlyBracesInHedColumn', { column: '{response_time}' }),
             standaloneTsvFiles[1].file,
+            { tsvLine: 2 },
           ),
         ],
         sidecars: [
@@ -637,30 +645,35 @@ describe('BIDS datasets', () => {
               column: 'response_time',
             }),
             combinedDatasets[0].file,
+            { tsvLine: 3 },
           ),
           BidsHedIssue.fromHedIssue(
             generateIssue('undefinedCurlyBraces', {
               column: 'response_time',
             }),
             combinedDatasets[1].file,
+            { tsvLine: 3 },
           ),
           BidsHedIssue.fromHedIssue(
             generateIssue('duplicateTag', {
               tag: 'Label/1',
             }),
             combinedDatasets[2].file,
+            { tsvLine: 3 },
           ),
           BidsHedIssue.fromHedIssue(
             generateIssue('duplicateTag', {
               tag: 'Label/1',
             }),
             combinedDatasets[2].file,
+            { tsvLine: 3 },
           ),
         ],
         hedColumn: [
           BidsHedIssue.fromHedIssue(
             generateIssue('curlyBracesInHedColumn', { column: '{response_time}' }),
             hedColumnDatasets[0].file,
+            { tsvLine: 2 },
           ),
         ],
         syntax: [
@@ -699,8 +712,8 @@ describe('BIDS datasets', () => {
         const tsvHedStrings = []
         for (const tsvFile of tsvFiles) {
           tsvFile.mergedSidecar.parseHedStrings(hedSchemas)
-          const tsvValidator = new BidsHedTsvValidator(tsvFile, hedSchemas)
-          const tsvHed = tsvValidator.parseHed()
+          const tsvValidator = new BidsHedTsvParser(tsvFile, hedSchemas)
+          const tsvHed = tsvValidator.parse()
           assert.isEmpty(tsvValidator.issues, 'TSV file failed to parse')
           tsvHedStrings.push(...tsvHed)
         }

diff --git a/validator/event/validator.js b/validator/event/validator.js
@@ -267,6 +267,10 @@ export class HedValidator {
    * @param {Object<string, (string|number[])>} parameters The error string parameters.
    */
   pushIssue(internalCode, parameters) {
+    const tsvLine = this.parsedString.tsvLine ?? this.parsedString.tsvLines
+    if (tsvLine) {
+      parameters.tsvLine = tsvLine
+    }
     this.issues.push(generateIssue(internalCode, parameters))
   }
 }