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

Simplify deltacode output #16

Closed
steven-esser opened this issue Nov 7, 2017 · 13 comments
Closed

Simplify deltacode output #16

steven-esser opened this issue Nov 7, 2017 · 13 comments

Comments

@steven-esser
Copy link
Contributor

For the csv output, it would be a better presentation if we simply included a single path value, instead of empty or repeated paths that are redundant.

@johnmhoran johnmhoran self-assigned this Nov 16, 2017
@steven-esser
Copy link
Contributor Author

@johnmhoran Plan of attack:

  1. Review + merge PR 19 fix to dict #20
  2. Modify Delta object to_dict function to return only minimal info (category and path for the most part)
  3. Modify CSV ouput function to handle new data structure
  4. Modify JSON output to Add header information that has been removed from DeltaCode.to_dict()

@steven-esser
Copy link
Contributor Author

Feel free to push your changes in a branch without opening a PR. This is easier and less noisy than constantly updating a PR.

If you have questions or would like me to expand in detail on any of the above items, let me know.

@johnmhoran
Copy link
Contributor

@MaJuRG I've modified Delta.to-dict() and the CSV output to handle the new data structure, and fixed the 9 failing tests as well. About to add missing headers to the JSON output. Here's an excerpt from the current JSON output file testing a set of test scans with 1 added file:

{
    "added": [
        {
            "category": "added", 
            "path": "a/a5.py"
        }
    ], 
    "removed": [], 
    "modified": [], 
    "unmodified": [
        { . . .

How do we want to handle the redundant category information? Replace the 4 category keys with a single key named deltas and a value containing a list of category/path key/value pairs? Example:

{
    "deltas": [
        {
            "category": "added", 
            "path": "a/a5.py"
        },
        {
            "category": "unmodified", 
            "path": "a/a1.py"
        } . . .

Also, I assume we want the missing header info added to the top of the JSON output file. If so, I think that means adding values to the top of the incoming OrderedDict created by DeltaCode.to_dict(). Do I have that right??

johnmhoran added a commit that referenced this issue Nov 18, 2017
  * 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>
@johnmhoran
Copy link
Contributor

@MaJuRG I just committed and pushed my work to date so you can vet including in connection with my recent questions.

@steven-esser
Copy link
Contributor Author

We can keep the redundant categories around for now. If you recall our conversation yesterday, the delta object's category field will no longer match the Deltas dict category keys once we add license/copyright information.

@johnmhoran
Copy link
Contributor

I do recall, e.g., license_change. Thanks @MaJuRG . When you have a chance, can you sketch out an example excerpt of how you see the future JSON structure?

Re the 2nd question, do we want to add the missing header info to the top of the incoming OrderedDict. If yes, how does one do that? I've seen 2 approaches: rewrite the OrderedDict -- said to be slow but relatively straightforward -- or write a function to prepend.

Finally, once I finish this, what shall I tackle next?

@johnmhoran
Copy link
Contributor

@MaJuRG I can add the version header from inside the JSON function by moving the variable from __init__.py to cli.py, but adding the stats from DeltaCode.get_stats() has eluded me so far. Do we need to restore that to the DeltaCode.to_dict() method in order to add it to the JSON output file?

@johnmhoran
Copy link
Contributor

@MaJuRG I'm able to add the stats by passing new and old to generate_json and calling the stats like this: data['deltacode_stats'] = DeltaCode(new, old).get_stats(). The 2 headers (version and stats) have been added to the JSON file, though they appear at the bottom rather than the top (per my question above re adding to the top).

@steven-esser
Copy link
Contributor Author

steven-esser commented Nov 18, 2017

a = OrderedDict([
    ('header', header_info),
    ('deltacode_stats' deltacode.get_stats()),
    ('deltas', deltacode.to_dict())
])

pass the deltacode object into generate_json instead of the data dict and just call to_dict inside the csv function at the right place.

@steven-esser
Copy link
Contributor Author

Shouldnt need to move version to cli.py. Just import deltacode at the top and reference the version like: deltacode.__version__

@johnmhoran
Copy link
Contributor

Very nice. Thanks, @MaJuRG .

My only open issue: the list comprehension you suggested -- still digging into how to include the 3 variable assignments and the tuple_list.append() operation currently inside the 2nd of the nested for loops.

@johnmhoran
Copy link
Contributor

@MaJuRG I believe I've answered my last question. Spent time yesterday trying to figure out how to address the double-nested multiple variable assignments and the append operation inside a list comprehension. Nothing seemed to work, no hints from my research.

Took a fresh look this morning and realized that I've seen this before in simpler form: initializing variables earlier than necessary. When all is said and done, it's just an append operation.

And this:

    tuple = ()
    tuple_list = []
    deltas = data

    for delta in deltas:
        category = delta
        for f in deltas[delta]:
            category = f['category']
            path = f['path']
            tuple = (category, path)
            tuple_list.append(tuple)

. . . can be replaced with this:

    deltas = data
    tuple_list = [(f['category'], f['path']) for delta in deltas for f in deltas[delta]]

All 79 tests pass. I want to do a little command-line testing just to be sure, and if all looks good, will clean up the code, commit, push and open a PR.

johnmhoran added a commit that referenced this issue Nov 20, 2017
Signed-off-by: John M. Horan <johnmhoran@gmail.com>
johnmhoran added a commit that referenced this issue Nov 21, 2017
  * Remove call to Delta.to_dict().

Signed-off-by: John M. Horan <johnmhoran@gmail.com>
@steven-esser
Copy link
Contributor Author

#21 merged, closing this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants