Fixed the warning that had to do with issue #1557#1604
Conversation
The warning messages have been updated to be the correct warning for this issue and the code has been fixed to properly display the warning when the sheet is blank.
dondi
left a comment
dondi
left a comment
There was a problem hiding this comment.
Similar note to last time, plus we also have the failing test to look into. Examining the specific test that failed, the failure is due to a mismatched warning count, which this PR could have introduced due to the new warning. We can examine the result more closely when we meet in order to determine whether this calls for a change in the test or genuinely flags wrong behavior
| errorDescription: prefix | ||
| ? [ | ||
| `There were no genes and values supplied in the ${sheetName} in the exported workbook.`, | ||
| `GRNsight is checking because a ${sheetName.split("_")[1] + " " + sheetName.split("_")[2]} value should have been provided as GRNmap output,`, |
There was a problem hiding this comment.
We have a repeated split call here again, so we want to do the same consolidation as before in order to call the function just once
Further, there’s a mix of + concatenation and template strings—for consistency, the latter is favored in modern JavaScript
So overall, I’d change this as follows: (using nameSegment as the variable name for the split result—feel free to pick the right name)
| `GRNsight is checking because a ${sheetName.split("_")[1] + " " + sheetName.split("_")[2]} value should have been provided as GRNmap output,`, | |
| `GRNsight is checking because a ${nameSegment[1]} ${nameSegment[2]}`, | |
| 'value should have been provided as GRNmap output,', |
And as you can see, finally, you’ll want to continue to be mindful of string lengths so that they interact with the line length gracefully
These attention-to-detail factors get noticed in industry and will build certain impressions with human reviewers one way or another. Automated reviewers might not remember habitual feedback but from a global picture that would mean token expenditure (and their environmental impact) that could have been avoided. So converging on good habits is beneficial either way
| ].join(" ") | ||
| : [ | ||
| `The "${sheetName}" sheet was empty in the exported workbook.`, | ||
| `GRNsight is checking because a ${sheetName.split("_")[1] + " " + sheetName.split("_")[2]} value should have been provided as GRNmap output,`, |
Added the changes that cleaned up the format for the warning that had to do with file B in issue #1557
|
@dondi I've applied the proper changes that you suggested. The tests will still break because we need to decide how to approach the additional warnings which we can talk about in our meeting. |
Fixed the test file to properly test for blank additional sheets and correctly check to make sure that they display one warning instead of the original zero.
Forgot to add this file to the push that actually fixed the test file.
|
coverage: 81.798% (+0.03%) from 81.764% — alex/two_column_warnings_1557_b into beta |
3 participants
The warning messages have been updated to be the correct warning for this issue and the code has been fixed to properly display the warning when the sheet is blank.