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.

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

Merged
merged 2 commits into from
Jan 8, 2019

Conversation

stevewardle
Copy link
Contributor

@stevewardle stevewardle commented Jan 7, 2019

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 a rose_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)

@stevewardle stevewardle added this to the soon milestone Jan 7, 2019
@stevewardle stevewardle self-assigned this Jan 7, 2019
@matthewrmshin matthewrmshin modified the milestones: soon, next-release Jan 7, 2019
@stevewardle
Copy link
Contributor Author

stevewardle commented Jan 7, 2019

@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!

@matthewrmshin
Copy link
Member

As discussed off line, perhaps just state that you are in KGO mode or not. Using the diff syntax as an analogy...

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
...

@stevewardle
Copy link
Contributor Author

As discussed off line, perhaps just state that you are in KGO mode or not.

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)

@matthewrmshin
Copy link
Member

@stevewardle is this ready for review or do you intend to add some tests as well?

@stevewardle
Copy link
Contributor Author

@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

@matthewrmshin
Copy link
Member

@paulcresswell Please sanity check. Approve and merge if you can.

Copy link
Contributor

@paulcresswell paulcresswell left a 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.

@paulcresswell paulcresswell merged commit 53e988b into metomi:master Jan 8, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants