Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Dr2 2049 make number of displayed errors configurable #527

Merged
merged 5 commits into from
Jan 23, 2025
Merged

Dr2 2049 make number of displayed errors configurable #527

Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
80d547e
Make number of displayed errors configurable
techncl Jan 21, 2025
e3327e4
Remove braces/rename row method & make 'validate' method easier to read
techncl Jan 21, 2025
e3230d0
Replace 'Line' with 'Row'
techncl Jan 21, 2025
fd88798
Shorten error message, use 'errors' instead of lines & use colons for…
techncl Jan 22, 2025
5f889e6
Move number of errors higher up to the text box options
techncl Jan 22, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
43 changes: 34 additions & 9 deletions ...validator-ui/src/main/scala/uk/gov/nationalarchives/csv/validator/ui/CsvValidatorUi.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -136,7 +136,7 @@ object CsvValidatorUi extends SimpleSwingApplication {
}
}

private def validate(csvFilePath: String, csvEncoding: Charset, csvSchemaFilePath: String, csvSchemaEncoding: Charset, failOnFirstError: Boolean, pathSubstitutions: List[(String, String)], enforceCaseSensitivePathChecks: Boolean, progress: Option[ProgressCallback], validateEncoding: Boolean, skipFileChecks: Boolean, outputTextSuffix: String)(output: String => Unit) : Unit = {
private def validate(csvFilePath: String, csvEncoding: Charset, csvSchemaFilePath: String, csvSchemaEncoding: Charset, failOnFirstError: Boolean, pathSubstitutions: List[(String, String)], enforceCaseSensitivePathChecks: Boolean, progress: Option[ProgressCallback], validateEncoding: Boolean, potentialMaxNumOfLines: String, skipFileChecks: Boolean, outputTextSuffix: String)(output: String => Unit) : Unit = {

def toConsole(msg: String): Unit = Swing.onEDT {
output(msg)
Expand All @@ -145,12 +145,23 @@ object CsvValidatorUi extends SimpleSwingApplication {
var badLines = 0
var truncated = false

def rowCallback(row: ValidatedNel[FailMessage, Any]): Unit = row match {
val maxNumOfLines = Try(potentialMaxNumOfLines.toInt) match {
case Success(number) if number > 0 => number
case _ =>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think if we error finding the limit, we should, perhaps, just go ahead with the current default limit of 2000 instead of setting it to zero. Maybe get Steve's opinion as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reason I set it to 0 here is so that on line 154, it can do the conditional check maxNumOfLines > 0 and not run the validation (after it sends the error message on line 151). Wouldn't it be better to just return an error rather than running the validation?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My thinking is, it becomes more restrictive than previous version, also if that setting is missing, the application can still carry on and succeed, hence the comment. Perhaps get Steve's opinion as well.

toConsole("Error: Maximum number of lines to display must be a positive whole number starting from 1")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps a simpler message "should be more than zero" or something like that. Technically, "whole numbers" include zero

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah yes. 😄 I was going back and forth trying to make this message more succinct; yours is much better.

0
}
val safeToRunValidation = csvFilePath.nonEmpty && csvSchemaFilePath.nonEmpty && maxNumOfLines > 0

def rowCallback(maxBadLines: Int)(row: ValidatedNel[FailMessage, Any]): Unit = row match {

case Invalid(failures) =>
if (badLines > 2000) {
if (badLines >= maxBadLines) {
if (!truncated) {
toConsole("Too many errors/warnings, truncating")
toConsole(
s"...\n\nNote: Number of lines to display has reached the set limit of $maxBadLines; " +
"increase this limit and re-run in order to display more lines."
)
truncated = true
}
} else {
Expand All @@ -162,7 +173,7 @@ object CsvValidatorUi extends SimpleSwingApplication {
case _ =>
}

if(csvFilePath.nonEmpty && csvSchemaFilePath.nonEmpty) {
if(safeToRunValidation) {
val (status, cliExitCode) = CsvValidatorCmdApp.validate(
TextFile(Paths.get(csvFilePath), csvEncoding, validateEncoding),
TextFile(Paths.get(csvSchemaFilePath), csvSchemaEncoding),
Expand All @@ -172,7 +183,7 @@ object CsvValidatorUi extends SimpleSwingApplication {
trace = false,
progress,
skipFileChecks,
rowCallback
rowCallback(maxNumOfLines)
)

cliExitCode match {
Expand Down Expand Up @@ -397,6 +408,7 @@ object CsvValidatorUi extends SimpleSwingApplication {
settingsPanel.enforceCaseSensitivePathChecks,
Some(progress),
settingsPanel.validateUtf8,
settingsPanel.numOfLinesToDisplay,
skipFileChecks,
suffix
)
Expand Down Expand Up @@ -502,6 +514,8 @@ object CsvValidatorUi extends SimpleSwingApplication {
cbFailOnFirstError.tooltip = "Indicates whether to fail on the first error, or whether to collect all errors!"
private val cbValidateUtf8 = new CheckBox("Validate CSV for valid UTF-8 characters")
cbValidateUtf8.selected = true
private val tfDisplayLinesLabel = new Label("Maximum number of lines to display:")
private val tfDisplayLines = new TextField("2000", 5)
private val lblPathSubstitutions = new Label("Path Substitutions")
private val cbEnforceCaseSensitivePathChecks = new CheckBox("Enforce case-sensitive file path checks")
cbEnforceCaseSensitivePathChecks.tooltip = "Performs additional checks to ensure that the case of file-paths in the CSV file match those of the filesystem"
Expand Down Expand Up @@ -602,21 +616,31 @@ object CsvValidatorUi extends SimpleSwingApplication {

c.gridx = 0
c.gridy = 5
c.insets = new Insets(0, 8, 0, 0)
layout(tfDisplayLinesLabel) = c

c.gridx = 1
c.gridy = 5
c.insets = new Insets(0, 0, 0, 0)
layout(tfDisplayLines) = c

c.gridx = 0
c.gridy = 6
c.insets = new Insets(0,10,0,0)
layout(lblPathSubstitutions) = c

c.gridx = 0
c.gridy = 6
c.gridy = 7
c.gridwidth = 2
layout(spTblPathSubstitutions) = c

c.gridx = 0
c.gridy = 7
c.gridy = 8
c.anchor = Anchor.LineStart
layout(btnRemovePathSubstitution) = c

c.gridx = 1
c.gridy = 7
c.gridy = 8
c.anchor = Anchor.LastLineEnd
layout(btnAddPathSubstitution) = c
}
Expand All @@ -630,5 +654,6 @@ object CsvValidatorUi extends SimpleSwingApplication {
def pathSubstitutions: List[(String, String)] = tblPathSubstitutions.pathSubstitutions
def enforceCaseSensitivePathChecks: Boolean = cbEnforceCaseSensitivePathChecks.selected
def validateUtf8 : Boolean = cbValidateUtf8.selected
def numOfLinesToDisplay: String = tfDisplayLines.text.strip()
}
}