Modify CLI interface

[slightlynybbled]

See issue #60

From an interface standpoint, I mostly followed the recommendations of @formatc1702.

There are minor differences put in place for convenience and compatibility with click.

Windows example:

> python src\wireviz\wireviz.py examples\demo01.yml -o png -o svg

Linux example:

> python src/wireviz/wireviz.py examples/demo01.yml -o png -o svg

The proposed pull request also assumes some architectural changes within the package that the author may not be comfortable with. It is my opinion that the Harness object should be highly concerned with generating the correct outputs and less concerned with file output manipulations, which explains the structure that I chose. I simply chose to write the outputs from parse() at the highest level. I'm assuming that expanding the Harness() object to support formats as properties is the most straightforward way of expanding the package and that the idea of the Harness() object actually writing to files itself will become awkward as time goes on. The idea currently implemented as properties svg and png could easily be expanded to create other properties csv, html, etc and allow higher-level code to do the coordination and file io with returned data.

There are still some rocks in here, and I don't expect the PR to be accepted in its current form, but I do believe that it is easier to maintain and expand with this as the starting at this point.

[slightlynybbled]

Output of help file:

(venv) C:\_code\WireViz>python src\wireviz\wireviz.py --help
Usage: wireviz.py [OPTIONS] INPUT_FILE

  Take a YAML file containing a harness specification and, utilizing
  GraphViz, translate that specification into various output formats.
  Example:

      $>wireviz my_input_file.yml -o png -o svg

Options:
  -p, --prepend TEXT  a YAML file containing a library of templates and parts
                      that may be referenced in the `input_file`

  -o, --out TEXT      specify one or more output types to be generated;
                      currently supports "png" "svg"

  --help              Show this message and exit.

[17o2]

Cool!
Is it possible to list multiple output types without requiring -o for each one?
wireviz inputfile.yml -o png svg csv
If it's a limitation of click, I guess we're out of luck.

[slightlynybbled]

It is a limitation of click. I am fine with this limitation b/c it is very much in sync with other common CLIs (gcc, for instance). If you want to get around it, you might make an interpreted string instead:

$>wireviz inputfile.yml -o "png svg csv"

I don't love this, but it is your call.

[17o2]

The proposed pull request also assumes some architectural changes within the package that the author may not be comfortable with.

What I'm not comfortable with, is the current state of things. You raise a point I've had in the back of my mind for a while, so I'm very thankful for this proposal.
Ideally, we could offload all the output file handling into a separate module, wv_output.py or similar to separate the harness logic from all the CSV, TSV and HTML output.
@aakatz's functions from the bom_helper.py file submitted in #116 would also be well suited for inclusion in this module.

I have a proposal: @aakatz can submit a pull request for the improved CSV/TSV output to @slightlynybbled, who is welcome to refactor wireviz.py and Harness.py to address this very issue, including the new CSV/TSV.

If both contributors agree with this, I will close #116 without merging, in favor of including its commits as part of #123 (this)
In order not to further complicate things, I'd like to move the HTML output as-is to this new module... the HTML itself is a story for another day (#74).
Would this foreseeably cause trouble with any of the other open PRs?

[17o2]

If you want to get around it, you might make an interpreted string instead:

$>wireviz inputfile.yml -o "png svg csv"

I don't love this, but it is your call.

Nope, let's stick to your proposal then.

[slightlynybbled]

I'm ok with this. It jives nicely with separation of concerns, which I hold dear.

[17o2]

The way I see it, we would need a couple of functions:

• output_bom() based on generate_bom_outputs() from [refactor] Use the csv library to generate properly escaped TSV files #116
• output_html()

and some simpler functions:

• output_png()
• output_svg() <-- probably based on the existing png() and svg() properties
• output_gv() should be trivial

and a placeholder:

• output_pdf()

[slightlynybbled]

I made some pretty major changes:

• Harness class is no longer capable of generating files
• All file input/output are handled in main()
• Integrated bones of CSV file generation provided by @aakatz3 (significant refactor to provide data rather than write files)
• File generation path is much more consistent than previous versions
• Some refactor of build_examples.py to utilize main() (same function that the CLI will use)
• Removal of parse_file() function from wireviz.py (it was only being used by build_examples.py)
• Refactor of Harness.bom() and Harness.bom_list() methods to be private using the underscore convention.

I'm sure that there is still some cleanup to perform and it would be prudent to test

[17o2]

I won't be able to take a closer look today, but that all sounds very good.

Just be careful with build_examples.py as that will cause conflicts with #118... I suggest reverting any changes there, and rather working on @kvid's branch, submitting a PR to him directly so a controlled merge of both PRs is possible.

[kvid]

I recommend rebasing this branch onto the current dev to get rid of all the Merge entries in your history. It is easier to do this early. Otherwise, if you wait until preparing for merge-in (@formatc1702 seems to prefer rebasing into his dev), you might encounter more work dealing with conflicts then.

Some refactor of build_examples.py to utilize main()...

It seems several of your refactoring changes of build_examples.py was already done in #110, and I have several commits on top of that in #118. Please isolate the changes needed to interface main() and the extra file extension(s).

[kvid]

@aakatz3 wrote:

Whether YAML, arguments, and/or a central/default config file control the output, I'm not sure, but all seem decent (and ideally all three would be most flexible, even if the config has to be an additional argument)

One way to handle this combination of specifications, is to let the config file (if present) be the first of potentially several --prepend'ed YAML files. Then, later spesifications and finally any CLI options can override.

[slightlynybbled]

I'll wait until #110 and #118 are merged. My PR doesn't build as-is without changes to build_examples.py.

[17o2]

I'll wait until #110 and #118 are merged. My PR doesn't build as-is without changes to build_examples.py.

I'll set the status to Draft then, so I won't merge accidentaly before #118 is merged.

[17o2]

#118 has been merged into dev, so feel free to adapt this PR and submit it for review.

Before merging, I'd appreciate if you could compress the commit history a bit by squashing and thus removing intermediate commits, to keep the history clean. Otherwise, I would have to resort to squashing all PR commits into one, which isn't the best idea for a larger refactoring such as this one. Thanks!

[slightlynybbled]

I messed with this for longer than I care to admit... I don't know how to squash. I think that some things are broken... could use an assist.

[kvid]

I messed with this for longer than I care to admit... I don't know how to squash. I think that some things are broken... could use an assist.

If you can describe in more detail, what you think is broken, I might have a look to assist you. See also my description below on how you can rebase to squash commits together, and to untangle your branch history.

Do you normally use use command line git or some GUI client?

Personally, I normally use command line git together with gitk as a simple GUI client for visualizing branches and diffs between commits.

• First, I recommend using gitk or some other GUI client to get a grasp on what commits to combine, reorder, or eliminate.

• Then, I recommend running interactive rebase: https://stackoverflow.com/questions/2302736/trimming-git-commits-squashing-git-history/2302947#2302947

• As a recent example, you can look at my commits to PR Extended build examples #118 before I rebased: https://github.com/kvid/WireViz/commits/extended-build-examples-before-rebase

• I recommend creating a new branch at your starting point (as a bookmark), like I did: git branch extended-build-examples-before-rebase

• To deal with my last 15 commits, this command is used: git rebase -i HEAD~15

• Then, git showed me the commits in an editor:

pick 9ad3e13 Reduce code duplication by moving common code into a generic function
pick d4feae6 Add action to restore generated files from git repository
pick 64f6507 Add action to compare generated files against git repository
pick 099c202 Simplify code
pick ec29076 Make all actions honor the optional argument -generate
pick e3ad11a Move open_file_append() outside the if to avoid re-open
pick ba4b900 Avoid including readme in all file groups
pick 1ca8bd1 Simplify code
pick a9e7337 Rename some variables to better reflect their contents and relations
pick 58a54b2 Move test to include readme inside collect_filenames() function
pick f2a0db0 Improve status output by adding group name
pick be28803 Restructure the group dict initialization
pick d3b299b Rename -generate option to -g/--groups and add argument help
pick b4f9829 Move group loop into build_generated() for consistency
pick 253cb3a Add CLI option -c that allows comparing Graphviz output also

# Rebase 94b3ffa..253cb3a onto 94b3ffa (15 commands)
#
# Commands:
# p, pick = use commit
# r, reword = use commit, but edit the commit message
# e, edit = use commit, but stop for amending
# s, squash = use commit, but meld into previous commit
# f, fixup = like "squash", but discard this commit's log message
# x, exec = run command (the rest of the line) using shell
# d, drop = remove commit
#
# These lines can be re-ordered; they are executed from top to bottom.
#
# If you remove a line here THAT COMMIT WILL BE LOST.
#
# However, if you remove everything, the rebase will be aborted.
#
# Note that empty commits are commented out

• I decided to combine my first 4 commits into one, then combine my next 9 commits (except be28803) into another one, and then keep the rest as single commits. First, I moved the line with be28803 below the line with d3b299b (i.e. swapping the order of these two commits). Then I replaced the leading pick with squash (or simply s) on all lines containg commits I wanted to squash together with with the commit above, like this:

pick 9ad3e13 Reduce code duplication by moving common code into a generic function
squash d4feae6 Add action to restore generated files from git repository
squash 64f6507 Add action to compare generated files against git repository
squash 099c202 Simplify code
pick ec29076 Make all actions honor the optional argument -generate
squash e3ad11a Move open_file_append() outside the if to avoid re-open
squash ba4b900 Avoid including readme in all file groups
squash 1ca8bd1 Simplify code
squash a9e7337 Rename some variables to better reflect their contents and relations
squash 58a54b2 Move test to include readme inside collect_filenames() function
squash f2a0db0 Improve status output by adding group name
squash d3b299b Rename -generate option to -g/--groups and add argument help
pick be28803 Restructure the group dict initialization
pick b4f9829 Move group loop into build_generated() for consistency
pick 253cb3a Add CLI option -c that allows comparing Graphviz output also

• During the process, you are offered to edit the combined commit description of your squash commits, and handle any conflicts, e.g. when reordering commits.

• This is how my branch ended up: https://github.com/kvid/WireViz/commits/extended-build-examples

• Take a look at my combined commits and compare them with the individual commits in the old branch.

• If you e.g. have a commit that reverse another commit partially, or several commits that are intermediate steps to obtain a new feature that deserves a combined description, I recommend squashing them together.

• If you make a mistake or are not fully satisfied with the result, you can always go back to the old branch via the "bookmark" branch you created, and start all over.

• When satisfied, then I recommend rebasing your shorter branch onto the current dev branch (if you already have the official repo as a remote, you can skip the the first command below and replace forked_from with your remote name) :

git remote add forked_from https://github.com/formatc1702/WireViz.git
git fetch forked_from
git checkout -b official-dev forked_from/dev
git checkout dev
git branch my-dev-before-rebase-onto-official-dev
git rebase official-dev

• You probably can simplify the process above by skipping the two checkout commands and modify the last command to git rebase forked_from/dev, but I have not tried that. I prefer to have the local branch for easy testing anyway.

• During this process, you normally need to handle conflicts between commits added to your branch and commits added to forked_from/dev since you forked/branched out from it. Be aware that when some of these commits are merge commits, any conflicts you had to resolve during those merge commits might have to be resolved again when rebasing. Unless these merge commits have a description explaining how any conflicts were resolved, you might have to compare diffs in the old branch to find out how they were resolved.

• As before, if you make a mistake or are not fully satisfied with the result, you can always go back to the old branch via the my-dev-before-rebase-onto-official-dev branch you created, and start from there again.

• When satisfied, you need to force-push your rebased branch to your forked GitHub-repo (force-push is needed because history is rewritten during a rebase): git push -f

• As a final note for future work, I recommend creating a new feature/bugfix branch with a unique name each time you branch out from the current official dev branch to work on something you want to use in a new PR. Then you avoid confusions about keeping apart deviated branches linked to identical branch names in different remote repos as with your dev branch here.

[17o2]

That is some fantastic advice right there. Thank you @kvid.

[17o2]

Any progress on the commit restructuring? I'm afraid it's not possible for me or @kvid to check out the PR and do the squashing, without removing @slightlynybbled's authorship, which would be a shame. But maybe there is some way to help out that I'm not aware of?

[17o2]

Since there has not been any progress on this front, I will move this topic to the v0.3 milestone, since I want to get v0.2 released soon. This shouldn't be a huge deal-breaker, and it's no problem to release v0.3 soon if something happens here shortly after release.

[17o2]

@slightlynybbled , I hope you're well and I'm wondering whether you are able to keep working on getting this PR ready for a merge?

[slightlynybbled]

I'm going to close the pull request and do a fresh try later. Too much has happened and I don't think that the PR in its current form is easily salvageable.