Add switch to not store full text of scripts run #389
Conversation
There was a problem hiding this comment.
Thanks a lot for your contribution, @Peckham.
Could you please look throught the comments? Especially the ones where I think there is an actual negation too much. In light of that, do you think it makes sense to change the variable/flag to an affirmative named one?
| @@ -255,12 +255,23 @@ public IEnumerable<string> get_statements_to_run(string sql_to_run) | |||
|
|
|||
| public void record_script_in_scripts_run_table(string script_name, string sql_to_run, bool run_this_script_once, long version_id) | |||
| { | |||
| var hash = hash_generator.create_hash(sql_to_run, true); | |||
| if (!configuration.DoNotStoreScriptsRunText) | |||
There was a problem hiding this comment.
Isn't this one inverted? Shouldn't the sql_to_run be set to null if DoNotStoreScriptsRunText is set to true?
| } | ||
|
|
||
| public void record_script_in_scripts_run_errors_table(string script_name, string sql_to_run, string sql_erroneous_part, string error_message, string repository_version, string repository_path) | ||
| { | ||
| if (!configuration.DoNotStoreScriptsRunText) |
There was a problem hiding this comment.
Same comment. I think this one should not be negated?
| //do not store full text of run scripts | ||
| .Add("donotstorescriptsruntext", | ||
| "DoNotStoreScriptsRunText - This instructs RH to not store the full script text in the database. Defaults to false.", | ||
| option => configuration.DoNotStoreScriptsRunText = option != null) |
There was a problem hiding this comment.
Is it better to name it "Positive"? i.e. StoreScriptsRunText, and default it to true? I think double negations are a bit hard to relate to (see also my comments further down, I think there is a negation too much in the code).
See e.g. https://softwareengineering.stackexchange.com/a/196837 for a discussion on affirmative (isXXX) vs negative (IsNotXXX) names on boolean variables.
Feel free to disagree, by all means :)
There was a problem hiding this comment.
I do agree on the affirmative naming. That's what I had initially (hence the forgotten negations), but it didn't quite feel consistent with DoNotCreateDatabase and DoNotAlterDatabase. That's why I changed it to the negative name.
I'll try to update my PR tomorrow :)
There was a problem hiding this comment.
Fair point. Maybe we should keep it as "DoNot". There are arguments for making the "unordinary" run cases a "true" flag too, if you see what I mean. So I may be convinced in both directions, really. So, keep the negative form if you like. But, please have a look at the concerns with the double negations of the flag in the code. I think the logic is reversed (but I might be wrong)
There was a problem hiding this comment.
You're right about the double negations. I've fixed them in the latest commit
There was a problem hiding this comment.
Thanks. Tested it locally, seems to work great. Thanks a lot for your contribution!!
Provides a solution for #354 by adding a new configuration property