Adds --verify

[jspaezp]

Discussed here #153

The idea is to have a cli flag that works as --dry-run BUT returns a status 1 if any change would have been made on the files.

This PR is still a work in progress, right now its compatible with the behavior of the rest of the flags but I think we need to add a test case that verifies its behavior directly (2 files, one that gets changed, one that doesnt and make sure they exit with the correct error) an additional test I can think of is that no file should return status 1 if it has already been stripped (so .... step 1, verify it returns status 1, make sure no changes are made; step 2 run bstripout, step 3 verify that it returns status 0 on the modified file).

[kynan]

What's the reason for writing to a JSON formatted string here and below? Could you also just compare the dicts directly? Not necessarily objecting, just trying to understand why you've implemented it this way :)

[jspaezp]

I was afraid of instances where the dict is different but the serialization is the same; for instance if for some reason the strip zepellin converts a list to a tuple. Both are equivalent in json but not as python dicts. (It might also prevent some edge cases where copying vs deep cloning might be an issue)

Example

import json

d1 = {"a": 1, "b": [1,]}
d2 = {"a": 1, "b": (1,)}

print(json.dumps(d1) == json.dumps(d2))
# True
print(d2 == d1)
# False
print(json.dumps(d2))
# {"a": 1, "b": [1]}

(I am not 100% sure if my concerns are totally grounded, but felt better).

[kynan]

You are indeed correct that both strip_output and strip_zeppelin_output mutate the existing dict, so we'd probably need to create a deep copy for comparison purposes. I'd prefer that over the JSON serialization.

[kynan]

Suggested change

	any_local_change = process_notebook(input_stream, output_stream, args, extra_keys)
	any_change = any_change or any_local_change
	if process_notebook(input_stream, output_stream, args, extra_keys):
	any_change = True

Wouldn't this be easier?

[kynan]

Same question: why do you need to convert to JSON?

[jspaezp]

Inside the test my main concern was that line endings might be different between the input and the output files. (If my memory is not failing me...)

Although to be fair this might have been an overstep. since it actually changes the test ... it tests whether the content is equivalent instead of testing that the file is actually the same. (lmk if I should revert it here)

[kynan]

This should indeed not be required. When I investigated this I was going crazy cause there was a persistently failing test, until I realized this was due to some extra leading whitespace in one of the test notebook input files, which I fixed in 5077ab4. Can you please rebase your PR on main and then make those changes?

[jspaezp]

thanks for the fix, will do.

[kynan]

You are indeed correct that both strip_output and strip_zeppelin_output mutate the existing dict, so we'd probably need to create a deep copy for comparison purposes. I'd prefer that over the JSON serialization.

[kynan]

This should indeed not be required. When I investigated this I was going crazy cause there was a persistently failing test, until I realized this was due to some extra leading whitespace in one of the test notebook input files, which I fixed in 5077ab4. Can you please rebase your PR on main and then make those changes?