Skip to content

Commit 2d3cc74

Browse files
happy5214happy5214
authored
Merge pull request #144 from hed-standard/add-tsv-line-to-issues
Add TSV line numbers to TSV-related issue messages
2 parents 6961b2c + 7d149dc commit 2d3cc74

File tree

7 files changed

+117
-45
lines changed

7 files changed

+117
-45
lines changed

‎bids/types/issues.js

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -27,12 +27,23 @@ export class BidsIssue {
2727

2828
/**
2929
* Whether this issue is an error.
30+
*
3031
* @returns {boolean}
3132
*/
3233
isError() {
3334
return bidsHedErrorCodes.has(this.code)
3435
}
3536

37+
/**
38+
* Determine if any of the passed issues are errors.
39+
*
40+
* @param {BidsIssue[]} issues A list of issues.
41+
* @return {boolean} Whether any of the passed issues are errors (rather than warnings).
42+
*/
43+
static anyAreErrors(issues) {
44+
return issues.some((issue) => issue.isError())
45+
}
46+
3647
static generateInternalErrorPromise(error) {
3748
return Promise.resolve([new BidsIssue(106, null, error.message)])
3849
}

‎bids/validate.js

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -88,6 +88,6 @@ class BidsHedValidator {
8888
*/
8989
_pushIssues(issues) {
9090
this.issues.push(...issues)
91-
return issues.some((issue) => issue.isError())
91+
return BidsIssue.anyAreErrors(issues)
9292
}
9393
}

‎bids/validator/bidsHedColumnValidator.js

Lines changed: 12 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -55,8 +55,8 @@ export class BidsHedColumnValidator {
5555
* @private
5656
*/
5757
_validateFileHedColumn(eventFileData) {
58-
return eventFileData.hedColumnHedStrings.flatMap((hedString) =>
59-
this._validateFileHedColumnString(eventFileData, hedString),
58+
return eventFileData.hedColumnHedStrings.flatMap((hedString, rowIndexMinusTwo) =>
59+
this._validateFileHedColumnString(eventFileData, hedString, rowIndexMinusTwo + 2),
6060
)
6161
}
6262

@@ -65,10 +65,11 @@ export class BidsHedColumnValidator {
6565
*
6666
* @param {BidsEventFile} eventFileData The BIDS event file whose HED column is to be validated.
6767
* @param {string} hedString The string to be validated.
68+
* @param {number} rowIndex The index of this row in the TSV file.
6869
* @returns {BidsHedIssue[]} All issues found.
6970
* @private
7071
*/
71-
_validateFileHedColumnString(eventFileData, hedString) {
72+
_validateFileHedColumnString(eventFileData, hedString, rowIndex) {
7273
if (!hedString) {
7374
return []
7475
}
@@ -81,7 +82,9 @@ export class BidsHedColumnValidator {
8182
}
8283

8384
const [parsedString, parsingIssues] = parseHedString(hedString, this.hedSchemas)
84-
issues.push(...BidsHedIssue.fromHedIssues(Object.values(parsingIssues).flat(), eventFileData.file))
85+
issues.push(
86+
...BidsHedIssue.fromHedIssues(Object.values(parsingIssues).flat(), eventFileData.file, { tsvLine: rowIndex }),
87+
)
8588

8689
if (parsedString === null) {
8790
return issues
@@ -90,15 +93,18 @@ export class BidsHedColumnValidator {
9093
if (parsedString.columnSplices.length > 0) {
9194
issues.push(
9295
BidsHedIssue.fromHedIssue(
93-
generateIssue('curlyBracesInHedColumn', { column: parsedString.columnSplices[0].format() }),
96+
generateIssue('curlyBracesInHedColumn', {
97+
column: parsedString.columnSplices[0].format(),
98+
tsvLine: rowIndex,
99+
}),
94100
eventFileData.file,
95101
),
96102
)
97103
return issues
98104
}
99105

100106
const [, hedIssues] = validateHedString(parsedString, this.hedSchemas, options)
101-
const convertedIssues = BidsHedIssue.fromHedIssues(hedIssues, eventFileData.file)
107+
const convertedIssues = BidsHedIssue.fromHedIssues(hedIssues, eventFileData.file, { tsvLine: rowIndex })
102108
issues.push(...convertedIssues)
103109

104110
return issues

‎bids/validator/bidsHedTsvValidator.js

Lines changed: 55 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
import { BidsHedSidecarValidator } from './bidsHedSidecarValidator'
2-
import { BidsHedIssue } from '../types/issues'
2+
import { BidsHedIssue, BidsIssue } from '../types/issues'
33
import { BidsTsvEvent, BidsTsvRow } from '../types/tsv'
44
import { parseHedString } from '../../parser/main'
55
import ColumnSplicer from '../../parser/columnSplicer'
@@ -57,20 +57,70 @@ export class BidsHedTsvValidator {
5757
return this.issues
5858
}
5959

60-
const hedStrings = this.parseHed()
61-
if (hedStrings.length > 0) {
60+
const bidsHedTsvParser = new BidsHedTsvParser(this.tsvFile, this.hedSchemas)
61+
const hedStrings = bidsHedTsvParser.parse()
62+
this.issues.push(...bidsHedTsvParser.issues)
63+
if (!BidsIssue.anyAreErrors(this.issues)) {
6264
this.validateCombinedDataset(hedStrings)
6365
}
6466

6567
return this.issues
6668
}
6769

70+
/**
71+
* Validate the HED data in a combined event TSV file/sidecar BIDS data collection.
72+
*
73+
* @param {ParsedHedString[]} hedStrings The HED strings in the data collection.
74+
*/
75+
validateCombinedDataset(hedStrings) {
76+
const [, hedIssues] = validateHedDatasetWithContext(
77+
hedStrings,
78+
this.tsvFile.mergedSidecar.hedStrings,
79+
this.hedSchemas,
80+
{
81+
checkForWarnings: true,
82+
validateDatasetLevel: this.tsvFile.isTimelineFile,
83+
},
84+
)
85+
this.issues.push(...BidsHedIssue.fromHedIssues(hedIssues, this.tsvFile.file))
86+
}
87+
}
88+
89+
export class BidsHedTsvParser {
90+
/**
91+
* The BIDS TSV file being parsed.
92+
* @type {BidsTsvFile}
93+
*/
94+
tsvFile
95+
/**
96+
* The HED schema collection being parsed against.
97+
* @type {Schemas}
98+
*/
99+
hedSchemas
100+
/**
101+
* The issues found during parsing.
102+
* @type {BidsHedIssue[]}
103+
*/
104+
issues
105+
106+
/**
107+
* Constructor.
108+
*
109+
* @param {BidsTsvFile} tsvFile The BIDS TSV file being parsed.
110+
* @param {Schemas} hedSchemas The HED schema collection being parsed against.
111+
*/
112+
constructor(tsvFile, hedSchemas) {
113+
this.tsvFile = tsvFile
114+
this.hedSchemas = hedSchemas
115+
this.issues = []
116+
}
117+
68118
/**
69119
* Combine the BIDS sidecar HED data into a BIDS TSV file's HED data.
70120
*
71121
* @returns {ParsedHedString[]} The combined HED string collection for this BIDS TSV file.
72122
*/
73-
parseHed() {
123+
parse() {
74124
const tsvHedRows = this._generateHedRows()
75125
const hedStrings = this._parseHedRows(tsvHedRows)
76126

@@ -185,10 +235,9 @@ export class BidsHedTsvValidator {
185235
const splicedParsedString = columnSplicer.splice()
186236
const splicingIssues = columnSplicer.issues
187237
if (splicingIssues.length > 0) {
188-
this.issues.push(...BidsHedIssue.fromHedIssues(splicingIssues, this.tsvFile.file))
238+
this.issues.push(...BidsHedIssue.fromHedIssues(splicingIssues, this.tsvFile.file, { tsvLine }))
189239
return null
190240
}
191-
splicedParsedString.context.set('tsvLine', tsvLine)
192241

193242
return new BidsTsvRow(splicedParsedString, rowCells, this.tsvFile, tsvLine)
194243
}
@@ -262,22 +311,4 @@ export class BidsHedTsvValidator {
262311
)
263312
return null
264313
}
265-
266-
/**
267-
* Validate the HED data in a combined event TSV file/sidecar BIDS data collection.
268-
*
269-
* @param {ParsedHedString[]} hedStrings The HED strings in the data collection.
270-
*/
271-
validateCombinedDataset(hedStrings) {
272-
const [, hedIssues] = validateHedDatasetWithContext(
273-
hedStrings,
274-
this.tsvFile.mergedSidecar.hedStrings,
275-
this.hedSchemas,
276-
{
277-
checkForWarnings: true,
278-
validateDatasetLevel: this.tsvFile.isTimelineFile,
279-
},
280-
)
281-
this.issues.push(...BidsHedIssue.fromHedIssues(hedIssues, this.tsvFile.file))
282-
}
283314
}

‎common/issues/issues.js

Lines changed: 9 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -76,8 +76,7 @@ export class Issue {
7676
this.code = internalCode
7777
this.hedCode = hedCode
7878
this.level = level
79-
// Pre-convert all parameters except the substring bounds (an integer array) to their string forms.
80-
this.parameters = mapValues(parameters, (value, key) => (key === 'bounds' ? value : String(value)))
79+
this.parameters = parameters
8180
this.generateMessage()
8281
}
8382

@@ -94,9 +93,14 @@ export class Issue {
9493
* (Re-)generate the issue message.
9594
*/
9695
generateMessage() {
96+
// Convert all parameters except the substring bounds (an integer array) to their string forms.
97+
this.parameters = mapValues(this.parameters, (value, key) => (key === 'bounds' ? value : String(value)))
98+
9799
const bounds = this.parameters.bounds ?? []
98100
const messageTemplate = issueData[this.internalCode].message
99101
let message = messageTemplate(...bounds, this.parameters)
102+
103+
// Special parameters
100104
if (this.parameters.sidecarKey) {
101105
message += ` Sidecar key: "${this.parameters.sidecarKey}".`
102106
}
@@ -106,8 +110,11 @@ export class Issue {
106110
if (this.parameters.hedString) {
107111
message += ` HED string: "${this.parameters.hedString}".`
108112
}
113+
114+
// Append link to error code in HED spec.
109115
const hedCodeAnchor = this.hedCode.toLowerCase().replace(/_/g, '-')
110116
const hedSpecLink = `For more information on this HED ${this.level}, see https://hed-specification.readthedocs.io/en/latest/Appendix_B.html#${hedCodeAnchor}`
117+
111118
this.message = `${this.level.toUpperCase()}: [${this.hedCode}] ${message} (${hedSpecLink}.)`
112119
}
113120

‎tests/bids.spec.js

Lines changed: 25 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -1,13 +1,15 @@
11
import chai from 'chai'
22
const assert = chai.assert
3+
import cloneDeep from 'lodash/cloneDeep'
4+
35
import converterGenerateIssue from '../converter/issues'
46
import { generateIssue } from '../common/issues/issues'
57
import { SchemaSpec, SchemasSpec } from '../common/schema/types'
68
import { buildBidsSchemas, parseSchemasSpec } from '../bids/schema'
79
import { BidsDataset, BidsHedIssue, BidsIssue, validateBidsDataset } from '../bids'
810
import { bidsDatasetDescriptions, bidsSidecars, bidsTsvFiles } from './bids.spec.data'
911
import { parseHedString } from '../parser/main'
10-
import { BidsHedTsvValidator } from '../bids/validator/bidsHedTsvValidator'
12+
import { BidsHedTsvParser } from '../bids/validator/bidsHedTsvValidator'
1113

1214
describe('BIDS datasets', () => {
1315
/**
@@ -164,14 +166,14 @@ describe('BIDS datasets', () => {
164166
const expectedIssues = {
165167
all_good: [],
166168
all_bad: [
167-
BidsHedIssue.fromHedIssue(speedIssue, badDatasets[0].file),
168-
BidsHedIssue.fromHedIssue(maglevWarning, badDatasets[1].file),
169-
BidsHedIssue.fromHedIssue(speedIssue, badDatasets[2].file),
170-
BidsHedIssue.fromHedIssue(speedIssue, badDatasets[3].file),
171-
BidsHedIssue.fromHedIssue(maglevError, badDatasets[3].file),
172-
BidsHedIssue.fromHedIssue(converterMaglevError, badDatasets[3].file),
173-
BidsHedIssue.fromHedIssue(speedIssue, badDatasets[4].file),
174-
BidsHedIssue.fromHedIssue(maglevWarning, badDatasets[4].file),
169+
BidsHedIssue.fromHedIssue(cloneDeep(speedIssue), badDatasets[0].file, { tsvLine: 2 }),
170+
BidsHedIssue.fromHedIssue(cloneDeep(maglevWarning), badDatasets[1].file, { tsvLine: 2 }),
171+
BidsHedIssue.fromHedIssue(cloneDeep(speedIssue), badDatasets[2].file, { tsvLine: 3 }),
172+
BidsHedIssue.fromHedIssue(converterMaglevError, badDatasets[3].file, { tsvLine: 2 }),
173+
BidsHedIssue.fromHedIssue(cloneDeep(maglevError), badDatasets[3].file, { tsvLine: 2 }),
174+
BidsHedIssue.fromHedIssue(cloneDeep(speedIssue), badDatasets[3].file, { tsvLine: 3 }),
175+
BidsHedIssue.fromHedIssue(cloneDeep(maglevWarning), badDatasets[4].file, { tsvLine: 2 }),
176+
BidsHedIssue.fromHedIssue(cloneDeep(speedIssue), badDatasets[4].file, { tsvLine: 3 }),
175177
],
176178
}
177179
return validator(testDatasets, expectedIssues, specs)
@@ -202,30 +204,35 @@ describe('BIDS datasets', () => {
202204
tag: 'Boat',
203205
}),
204206
badDatasets[2].file,
207+
{ tsvLine: 5 },
205208
),
206209
BidsHedIssue.fromHedIssue(
207210
generateIssue('duplicateTag', {
208211
tag: 'Boat',
209212
}),
210213
badDatasets[2].file,
214+
{ tsvLine: 5 },
211215
),
212216
BidsHedIssue.fromHedIssue(
213217
generateIssue('invalidValue', {
214218
tag: 'Duration/ferry s',
215219
}),
216220
badDatasets[3].file,
221+
{ tsvLine: 2 },
217222
),
218223
BidsHedIssue.fromHedIssue(
219224
generateIssue('duplicateTag', {
220225
tag: 'Age/30',
221226
}),
222227
badDatasets[3].file,
228+
{ tsvLine: 2 },
223229
),
224230
BidsHedIssue.fromHedIssue(
225231
generateIssue('duplicateTag', {
226232
tag: 'Age/30',
227233
}),
228234
badDatasets[3].file,
235+
{ tsvLine: 2 },
229236
),
230237
BidsHedIssue.fromHedIssue(
231238
generateIssue('sidecarKeyMissing', {
@@ -482,7 +489,7 @@ describe('BIDS datasets', () => {
482489
const expectedIssues = {
483490
bad_tsv: [
484491
BidsHedIssue.fromHedIssue(
485-
generateIssue('illegalDefinitionContext', { string: '(Definition/myDef, (Label/Red, Green))' }),
492+
generateIssue('illegalDefinitionContext', { string: '(Definition/myDef, (Label/Red, Green))', tsvLine: 2 }),
486493
badTsvDatasets[0].file,
487494
),
488495
],
@@ -542,6 +549,7 @@ describe('BIDS datasets', () => {
542549
BidsHedIssue.fromHedIssue(
543550
generateIssue('curlyBracesInHedColumn', { column: '{response_time}' }),
544551
standaloneTsvFiles[1].file,
552+
{ tsvLine: 2 },
545553
),
546554
],
547555
sidecars: [
@@ -637,30 +645,35 @@ describe('BIDS datasets', () => {
637645
column: 'response_time',
638646
}),
639647
combinedDatasets[0].file,
648+
{ tsvLine: 3 },
640649
),
641650
BidsHedIssue.fromHedIssue(
642651
generateIssue('undefinedCurlyBraces', {
643652
column: 'response_time',
644653
}),
645654
combinedDatasets[1].file,
655+
{ tsvLine: 3 },
646656
),
647657
BidsHedIssue.fromHedIssue(
648658
generateIssue('duplicateTag', {
649659
tag: 'Label/1',
650660
}),
651661
combinedDatasets[2].file,
662+
{ tsvLine: 3 },
652663
),
653664
BidsHedIssue.fromHedIssue(
654665
generateIssue('duplicateTag', {
655666
tag: 'Label/1',
656667
}),
657668
combinedDatasets[2].file,
669+
{ tsvLine: 3 },
658670
),
659671
],
660672
hedColumn: [
661673
BidsHedIssue.fromHedIssue(
662674
generateIssue('curlyBracesInHedColumn', { column: '{response_time}' }),
663675
hedColumnDatasets[0].file,
676+
{ tsvLine: 2 },
664677
),
665678
],
666679
syntax: [
@@ -699,8 +712,8 @@ describe('BIDS datasets', () => {
699712
const tsvHedStrings = []
700713
for (const tsvFile of tsvFiles) {
701714
tsvFile.mergedSidecar.parseHedStrings(hedSchemas)
702-
const tsvValidator = new BidsHedTsvValidator(tsvFile, hedSchemas)
703-
const tsvHed = tsvValidator.parseHed()
715+
const tsvValidator = new BidsHedTsvParser(tsvFile, hedSchemas)
716+
const tsvHed = tsvValidator.parse()
704717
assert.isEmpty(tsvValidator.issues, 'TSV file failed to parse')
705718
tsvHedStrings.push(...tsvHed)
706719
}

‎validator/event/validator.js

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -267,6 +267,10 @@ export class HedValidator {
267267
* @param {Object<string, (string|number[])>} parameters The error string parameters.
268268
*/
269269
pushIssue(internalCode, parameters) {
270+
const tsvLine = this.parsedString.tsvLine ?? this.parsedString.tsvLines
271+
if (tsvLine) {
272+
parameters.tsvLine = tsvLine
273+
}
270274
this.issues.push(generateIssue(internalCode, parameters))
271275
}
272276
}

0 commit comments

Comments
 (0)