16 simplify DeltaCode output - #21
Conversation
* Modify Delta object to_dict function to return only minimal info
(category and path).
* Modify CSV ouput function to handle new data structure.
* Refactor 9 failing tests and related test files.
Signed-off-by: John M. Horan <johnmhoran@gmail.com>
Signed-off-by: John M. Horan <johnmhoran@gmail.com>
|
|
||
| # output to csv | ||
| if csv_file: | ||
| generate_csv(data, csv_file) |
There was a problem hiding this comment.
We pass the whole delta object to generate_csv(), like we did for generate_json() to keep things similar.
| tuple = (category, new, old, new_filename, old_filename, new_sha1, | ||
| old_sha1, new_size, old_size, new_type, old_type, new_orig, old_orig) | ||
| tuple_list.append(tuple) | ||
| tuple_list = [(f['category'], f['path']) for delta in deltas for f in deltas[delta]] |
There was a problem hiding this comment.
tuple_list is a poor variable choice. Reading the list comprehension, we know that it is a list of tuples; no need to be redundant here.
There was a problem hiding this comment.
Instead, we can skip the varibles all together and just do the list comprehension on line 28
There was a problem hiding this comment.
and you should really use .get() syntax when accessing dictionaries.
| containing the primary information from the Delta objects. | ||
| """ | ||
| # TODO: Add json file headers here | ||
| json_dict = OrderedDict([ |
There was a problem hiding this comment.
same here, json_dict is a poor variable choice.
Something like output is all that is needed.
| generate_json(delta, json_file) | ||
| # print to stdout | ||
| else: | ||
| print(json.dumps(data, indent=4)) |
There was a problem hiding this comment.
We need to change this to print(json.dumps(delta.to_dict()), like the rest of our output functions.
Signed-off-by: John M. Horan <johnmhoran@gmail.com>
| """ | ||
| deltas = data | ||
| tuple_list = [(f['category'], f['path']) for delta in deltas for f in deltas[delta]] | ||
| output = delta.to_dict() |
There was a problem hiding this comment.
Looking closer at our csv function, we should probably NOT call to_dict() in the csv function now that we are passing the whole delta object. This is just unnecessary overhead and will cause our program to run slower.
| csv_out = csv.writer(out) | ||
| csv_out.writerow(['Type of delta', 'Path']) | ||
| for row in tuple_list: | ||
| for row in [(f.get('category'), f.get('path')) for d in output for f in output.get(d)]: |
There was a problem hiding this comment.
This means we should convert this line to something like:
for row in [f.category, f.path for d in delta.deltas for f in delta.deltas.get(d)]:
There was a problem hiding this comment.
@MaJuRG It seems to me that we have to call the Delta object's to_dict() method somewhere if we want to display the category and path values defined in that method. The following works:
for row in [(f.to_dict().get('category'), f.to_dict().get('path')) for d in delta.deltas for f in delta.deltas.get(d)]:
Is this what you had in mind? Let me know if you want me to push this first before you review.
There was a problem hiding this comment.
@johnmhoran We should not have to do that. category and path are fields in our delta object that we can access normally.
There was a problem hiding this comment.
to_dict() is only required for json output because dumping as a json requires it.
There was a problem hiding this comment.
Think about how you would access the path value in a delta object. What line of code would that be? You certainly do not need to convert to dict before being able to access an object field value(s).
There was a problem hiding this comment.
@MaJuRG We can access the path value of a new_file and/or an old_file if one or both exist, e.g., f.new_file.path. However, if we call a new_file for a delta where a file has been removed, we get a NoneType error, and similar result if we call an old_file where a file has been added. The Delta.to_dict() method handles these alternative scenarios, and I think we must as well if we're calling method with something like f.new_file.path.
. . . I'll be damned -- this seems to work:
for row in [(f.category, f.new_file.path if f.category == 'added' else f.old_file.path) for d in delta.deltas for f in delta.deltas.get(d)]:
Am I getting warm?
There was a problem hiding this comment.
@johnmhoran Good point about the to_delta() handling the different cases.
Two things:
- You are on the right track. The line you posted above looks good, except (and this is kind of personal preference) that I would do it the opposite logic, just so we are using the
new_file.pathwhenever able (to me this makes more sense from a user perspective):
for row in [(f.category, f.old_file.path if f.category == 'removed' else f.new_file.path) for d in delta.deltas for f in delta.deltas.get(d)]:
- IF you run into any bugs or strange output, you may just have to implement similar logic from
delta.to_dict()into this function. But, if you find the above line(s) work well and handle everything, this is probably OK for now.
There was a problem hiding this comment.
@MaJuRG Your approach works as well -- all 79 tests pass with both approaches. I'll commit and push shortly with your version. (I get and agree with your point re the user's perspective.)
* Remove call to Delta.to_dict(). Signed-off-by: John M. Horan <johnmhoran@gmail.com>
|
looks good, merging |
No description provided.