Allow percent % in path and file names #561#601
Conversation
HEdingfield
left a comment
HEdingfield
left a comment
There was a problem hiding this comment.
Were you able to test this with the files included in this issue?
| tabulationHandler = | ||
| new FileHandler(outputPath, LOG_FILE_MAX_SIZE_BYTES, TABULATION_LOG_FILE_COUNT, true); | ||
| // replace any instances of % with %% because the FileHandler pattern generator requires this | ||
| tabulationHandler = new FileHandler(outputPath.replace("%", "%%"), |
There was a problem hiding this comment.
Would this work as intended if a file name has a double percentage in the name?
There was a problem hiding this comment.
Yep, I tried this with multiple percent characters and that part works, but as I'm checking again I realize I broke the %g functionality here so it's not quite correct yet. Will push a fix for that.
There was a problem hiding this comment.
Just pushed a fix for this bit. Go ahead and get hog-wild with your path names.
|
@HEdingfield + @andyanderson yep - was able to tabulate: 2022-06-06 18:53:47 PDT INFO: Round: 4 Congratulations to Strawberry. |
HEdingfield
left a comment
HEdingfield
left a comment
There was a problem hiding this comment.
Big up the strawberry.
| tabulationHandler.setLevel(Level.FINE); | ||
| logger.addHandler(tabulationHandler); | ||
| info("Tabulation logging to: %s", outputPath.replace("%g", "*")); | ||
| info("Tabulation logging to: %s", tabulationLogPattern.replace("%g", "0")); |
There was a problem hiding this comment.
Why this change? It seems like it makes it look like it's always pointing to log index 0 when that may not actually be the case.
There was a problem hiding this comment.
I believe this is an improvement over the current message. At the time it's printed it's accurate and it is less confusing than the current asterisk message, which might mean something to folks acquainted with the notion of wildcards, but not to our users.
There was a problem hiding this comment.
I guess this begs the question of why we even include the log index at all if we know it's always going to be 0? I assume it serves some purpose and, if so, I think we should keep this as-is. We've already got an established precedent of using * in a log name for the execution log message, and I think it's a pretty well-understood concept.
In any case, this PR looks good, thanks for fixing this!
| tabulationHandler.setLevel(Level.FINE); | ||
| logger.addHandler(tabulationHandler); | ||
| info("Tabulation logging to: %s", outputPath.replace("%g", "*")); | ||
| info("Tabulation logging to: %s", tabulationLogPattern.replace("%g", "0")); |
There was a problem hiding this comment.
I guess this begs the question of why we even include the log index at all if we know it's always going to be 0? I assume it serves some purpose and, if so, I think we should keep this as-is. We've already got an established precedent of using * in a log name for the execution log message, and I think it's a pretty well-understood concept.
In any case, this PR looks good, thanks for fixing this!
2 participants
No description provided.