-
Notifications
You must be signed in to change notification settings - Fork 53
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.
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
Fix overzealous informational message about KGO in grepper class #2275
Fix overzealous informational message about KGO in grepper class #2275
Conversation
@matthewrmshin There's just one thing I'm trying to decide here... do you think the message itself is potentially misleading? It says "No KGO files are present"... which I fear might sometimes come across to the user as saying that there were meant to be KGO files but none have been found. The intent though is to convey that the app has specified that there are no KGO files (and the output is just repeating this) So perhaps it might usefully be changed to something more like "App indicates none of the input files are KGO" or just "App specifies no KGO" or even "This test does not compare against KGO"... any thoughts? Edit - and a case in point that after posting this @paulcresswell pointed out that "This test does not compare against KGO" would certainly be bad as well, for obvious reasons! |
As discussed off line, perhaps just state that you are in KGO mode or not. Using the If you are in KGO mode, you will state that the expected output is from a KGO: --- out1 (KGO)
out2
... If you are not in KGO mode, you will simply do: --- out1
out2
... |
I think that makes sense... ultimately the line stating that "No KGO is present" or similar is not really relevant or useful information; if one of the files is KGO it will say so, if not then it doesn't actually matter whether that's because the setting was blank or not specified (just that none of the files are KGO) |
@stevewardle is this ready for review or do you intend to add some tests as well? |
Go for it, thanks... I'm not sure what value testing would add here as not much has actually changed |
@paulcresswell Please sanity check. Approve and merge if you can. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems reasonable to me. I note that "the behaviour if the kgo_file
input is completely omitted and the behaviour if it is provided but left completely empty" (as in the OP) are still the same, but from talking to Steve I understand that this isn't considered a problem.
There's a bit of logic in the way the
grepper
class deals with the specification of the KGO files which leads to it spamming a message in an unhelpful way...Currently the behaviour if the
kgo_file
input is completely omitted and the behaviour if it is provided but left completely empty are exactly the same. As a result arose_ana
task which omits the setting because it doens't care about KGO at all (it may simply not use the concept of KGO) will print the message "No KGO files are present" for every single comparison made. Clearly this is undesirable...The solution is to reshuffle the logic so that the message is only printed in the case of the input being explicitly set to blank (with complete omission printing no messags at all)