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

Extended build examples #118

Merged
merged 7 commits into from
Jul 27, 2020
Merged

Conversation

kvid
Copy link
Collaborator

@kvid kvid commented Jul 21, 2020

This PR builds on top of #110 and add some new features:

  • Add compare action to compare generated files (except those generated by Graphviz unless using -c) against git repository.
  • Add restore action to restore generated files from git repository.
  • Make all actions honor the optional argument -g. This make it possible to select the group of files to use for any action by following the action argument with -g and then the space separated group names.

Edit: It should not break the existing build and clean actions, but their algorithms to collect files are slightly adjusted so they can use a common function to reduce code duplication.

These new features are useful for #63, but does not close it (collecting corner cases are also needed).

Edit 2: A few extra changes seen by the user:

  • The CLI option -generate is renamed to -g/--groups and some argument help is added.
  • The group name is included in the status output as the path is equal for two groups, and now it contains {description} {groupkey} in {path} e.g. Building demos in /path/WireViz/examples.

Usage:

  • Run python build_examples.py to build generated files in all groups.
  • Run python build_examples.py compare to compare generated files in all groups against the last commit.
  • Run python build_examples.py clean to delete generated files in all groups.
  • Run python build_examples.py restore to restore generated files in all groups from the last commit.
  • Edit 4: Append -c or --compare-graphviz-output to the compare command above to also compare the Graphviz output (default: False).
  • Append -g or --groups followed by space separated group names to any command above, and the set of generated files affected by the command will be limited to the selected groups.
  • Run python build_examples.py -h or with --help to print the generated help. The auto-generated help text is not the best, but better than maintaining it manually. I wonder if click as recommended in [feature] Only output the files that user requests #60 (comment) could improve this?

Edit 3: This PR was originally built on top of #110, but is now rebased onto dev to prepare merge-in. The single commit from #110 that my commits depend on, is therefore now included here. Some of my commits are squashed together. The original branch before squash-rebasing is available as kvid:extended-build-examples-before-rebase.

Personally, I want to use python build_examples.py followed by python build_examples.py compare as a regression test to easily see what has changed in the output files just re-generated compared to the latest commit. In addition, I want to use python build_examples.py restore just before committing to avoid unwanted changes from e.g. a different Graphviz version. Hopefully, others also find this useful.

@formatc1702
Copy link
Collaborator

I will check out and test carefully later. But this looks like some badass git scripting, and I get the feeling it will be very nice to work with 😎

@kvid
Copy link
Collaborator Author

kvid commented Jul 21, 2020

There might be a minor issue about the readme beeing part of all three groups of generated files for all actions except build. It should not be critical, and I have an idea how to fix it, but I probably have to wait until late this evening to have time for testing.

@formatc1702
Copy link
Collaborator

I'll wait until you fix it, it's not urgent.

@kvid
Copy link
Collaborator Author

kvid commented Jul 22, 2020

Should I rename the -generate command line argument to --groups with -g as an alias (if possible), or might that break some code that call this script?

@formatc1702
Copy link
Collaborator

Should I rename the -generate command line argument to --groups with -g as an alias (if possible), or might that break some code that call this script?

I have no preference one way or the other. Let me know when your PR is ready, then I can merge into mine and both unto dev :)

@formatc1702
Copy link
Collaborator

formatc1702 commented Jul 24, 2020

Honestly, with all your hard work, it probably makes sense to squash and merge all your commits directly into dev...

  • makes multiple merging unnecessary
  • keeps the commit history simple (only one new commit)
  • retains you @kvid as the author of the squashed commit

Just let me know when you are ready, then I can test and do as described, if you think it's OK.

@kvid
Copy link
Collaborator Author

kvid commented Jul 24, 2020

@formatc1702 See the updated PR description for a summary of major changes. In addition, I have changed a lot of internal stuff that are better described in the short descriptions of each commit. The ?w=1 trick you told me about might also be useful. Do you think it's ready now?

@kvid
Copy link
Collaborator Author

kvid commented Jul 24, 2020

I can rebase my stuff (together with your bugfix/build-examples) onto the current dev if that will help you. Is it possible to include all the commit descriptions in one large description if squash-ing is needed?

@formatc1702
Copy link
Collaborator

I can rebase my stuff (together with your bugfix/build-examples) onto the current dev if that will help you. Is it possible to include all the commit descriptions in one large description if squash-ing is needed?

If you think it is valuable to have the complete commit history of this development in the final merge to dev, I can keep it as-is (14 commits, as of now). If not, I can edit the commit message before squashing, to anything you like :)

@formatc1702
Copy link
Collaborator

I can also edit this PR to point to dev as a target, as opposed to bugfix/build-examples, I'll try that now (shouldn't affect you at all)

@kvid
Copy link
Collaborator Author

kvid commented Jul 24, 2020

I dont' think we need all 14 in the history, but I would suggest more than one. I can squash them together to a smaller set with what I think might be useful descriptions and force-push them here again. However, I will respect your decision on what you want to keep in the main history.

I can also edit this PR to point to dev as a target, as opposed to bugfix/build-examples, I'll try that now (shouldn't affect you at all)

What will then happen to the commits on your bugfix/build-examples that I have built upon?

@formatc1702
Copy link
Collaborator

Feel free to rebase onto dev, force-push and restructure any way you want, and I will preserve your commits :)
I think the changes from my previous commits will just become part of yours, which doesn't matter to me.

@formatc1702 formatc1702 changed the base branch from bugfix/build-examples to dev July 24, 2020 18:39
@kvid kvid force-pushed the extended-build-examples branch 3 times, most recently from 9857eee to 67f5e20 Compare July 24, 2020 20:53
@kvid
Copy link
Collaborator Author

kvid commented Jul 24, 2020

@formatc1702 I have squashed together those commits that belongs together with a common description, reducing the count from 14 to 4 commits (in addition to your first commit from #110), and updated the PR description. The other commits from #110 was not needed, and I'm not sure if the commit Remove upload action is what we want. After reading the comments from @aakatz3 in #110, and downloading the artifacts in examples-and-tutorials.zip as he describes, I'm leaning towards that this doesn't change what is committed, but might be useful for downloading files generated in an environment independent of each developer. What do you think?

@formatc1702
Copy link
Collaborator

I guess it doesn't hurt to keep the .zip file, feel free to put it back in.
Thanks for the commit reorganizing!

@formatc1702 formatc1702 mentioned this pull request Jul 25, 2020
@kvid
Copy link
Collaborator Author

kvid commented Jul 25, 2020

@formatc1702 wrote:

I guess it doesn't hurt to keep the .zip file, feel free to put it back in.
Thanks for the commit reorganizing!

Then this PR is ready for merge-in if it passes your review. The commit Remove upload action is not included in my branch.

Edit: As you can see, I added an extra CLI option to be able to select the already prepared functionality to also compare the Graphviz output. It might be useful when the Graphviz version hasn't changed, e.g. in your own merge/rebase process. Please review when you have the time.

@formatc1702
Copy link
Collaborator

formatc1702 commented Jul 26, 2020

I've started testing the code, and I've found that git commands currently don't work if the file path contains spaces, at least under MacOS (10.15)

$ python build_examples.py compare --groups demos
Comparing demos in /Users/daniel/Projects/1 Git Projects/WireViz/examples
  git --no-pager diff /Users/daniel/Projects/1 Git Projects/WireViz/examples/demo01.bom.tsv
fatal: ambiguous argument '/Users/daniel/Projects/1': unknown revision or path not in the working tree.
Use '--' to separate paths from revisions, like this:
'git <command> [<revision>...] -- [<file>...]'

The build and clean options seem to work fine :)


input_extensions = ['.yml']
generated_extensions = ['.gv', '.png', '.svg', '.html', '.bom.tsv']
extensions_not_from_graphviz = [ext for ext in generated_extensions if ext[-1] == 'v']
Copy link
Collaborator

@formatc1702 formatc1702 Jul 26, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line is a bit cryptic (or maybe I'm just tired)... could you explain the logic? Thanks!

[Edit]: got it, but it was a head-scratcher at first!

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the interest of readability, maybe listing the extensions_not_from_graphviz explicitly is better than relying on the letter v ;-)

The fact that .gv is marked as "not from GraphViz" is ironic, but understandable.

Maybe shuffle the logic around a bit and have

graphviz_generated_extensions = ['.png', '.svg', '.html']`
other_extensions = ['.gv', '.bom.tsv']  # or some better variable name

?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I forgot to put a comment here about this temporary hack. I collect all generated_extensions that ends with a 'v' to filter out the Graphviz output, but I can do it in a more readable way instead.

Comment on lines +124 to +133
parser = argparse.ArgumentParser(description='Wireviz Example Manager',)
parser.add_argument('action', nargs='?', action='store',
choices=['build','clean','compare','restore'], default='build',
help='what to do with the generated files (default: build)')
parser.add_argument('-c', '--compare-graphviz-output', action='store_true',
help='the Graphviz output is also compared (default: False)')
parser.add_argument('-g', '--groups', nargs='+',
choices=groups.keys(), default=groups.keys(),
help='the groups of generated files (default: all)')
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is by no means mandatory, but if we're using the Click package for CLI parsing in wireviz.py, maybe it makes sense to use it here too, for consistency?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree, that's why I wrote in the PR description:

  • Run python build_examples.py -h or with --help to print the generated help. The auto-generated help text is not the best, but better than maintaining it manually. I wonder if click as recommended in [feature] Only output the files that user requests #60 (comment) could improve this?

I will have a look at that.

Copy link
Collaborator Author

@kvid kvid Jul 26, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think I have the time to complete this today. Is it better to finish this PR without click?

I have looked into click, but I didn't find a way to implement the --groups option. The documentation is not very detailed. Here is what I have tried so far, but I also got a lot of errors (e.g. SyntaxError: invalid syntax) I couldn't figure out right now:

----------------------------------- setup.py -----------------------------------
index 09621bb..cc5a3da 100644
@@ -24,6 +24,7 @@ setup(
     install_requires=[
         'pyyaml',
         'graphviz',
+        'click',
         ],
     license='GPLv3',
     keywords='cable connector hardware harness wiring wiring-diagram wiring-harness',
@@ -31,7 +32,10 @@ setup(
     package_dir={'': 'src'},
     packages=find_packages('src'),
     entry_points={
-        'console_scripts': ['wireviz=wireviz.wireviz:main'],
+        'console_scripts': [
+            'wireviz=wireviz.wireviz:main',
+            'build_examples=wireviz.build_examples:cli',
+            ],
         },
     classifiers=[
         'Development Status :: 4 - Beta',

------------------------ src/wireviz/build_examples.py ------------------------
index 10a4b1b..97c2726 100755
@@ -4,6 +4,7 @@
 import argparse
 import sys
 import os
+import click
 from pathlib import Path
 
 script_path = Path(__file__).absolute()
@@ -48,6 +49,12 @@ def collect_filenames(description, groupkey, ext_list):
     return sorted([filename for pattern in patterns for filename in path.glob(pattern)])
 
 
+@click.group()
+cli()
+    pass
+
+
+@cli.command()
 def build_generated(groupkeys):
     for key in groupkeys:
         # preparation
@@ -87,6 +94,7 @@ def build_generated(groupkeys):
                     out.write(f'[Source]({yaml_file.name}) - [Bill of Materials]({yaml_file.stem}.bom.tsv)\n\n\n')
 
 
+@cli.command()
 def clean_generated(groupkeys):
     for key in groupkeys:
         # collect and remove files
@@ -96,6 +104,7 @@ def clean_generated(groupkeys):
                 os.remove(filename)
 
 
+@cli.command()
 def compare_generated(groupkeys, include_graphviz_output = False):
     compare_extensions = generated_extensions if include_graphviz_output else extensions_not_containing_graphviz_output
     for key in groupkeys:
@@ -106,6 +115,7 @@ def compare_generated(groupkeys, include_graphviz_output = False):
             os.system(cmd)
 
 
+@cli.command()
 def restore_generated(groupkeys):
     for key in groupkeys:
         # collect input YAML files
@@ -147,4 +157,4 @@ def main():
 
 
 if __name__ == '__main__':
-    main()
+    cli()

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps the discussion on #123 might help you. It seems click requires one -g for each group you want to include. Not sure if it's a limitation, or a feature, and what reason it might have.

@kvid
Copy link
Collaborator Author

kvid commented Jul 26, 2020

I've started testing the code, and I've found that git commands currently don't work if the file path contains spaces, at least under MacOS (10.15)

$ python build_examples.py compare --groups demos
Comparing demos in /Users/daniel/Projects/1 Git Projects/WireViz/examples
  git --no-pager diff /Users/daniel/Projects/1 Git Projects/WireViz/examples/demo01.bom.tsv
fatal: ambiguous argument '/Users/daniel/Projects/1': unknown revision or path not in the working tree.
Use '--' to separate paths from revisions, like this:
'git <command> [<revision>...] -- [<file>...]'

Thank's for telling me immediately. I guess double quotes around the filenames should help. Can you try this command manually in MacOS?
git --no-pager diff "/Users/daniel/Projects/1 Git Projects/WireViz/examples/demo01.bom.tsv"

@formatc1702
Copy link
Collaborator

Thank's for telling me immediately. I guess double quotes around the filenames should help. Can you try this command manually in MacOS?
git --no-pager diff "/Users/daniel/Projects/1 Git Projects/WireViz/examples/demo01.bom.tsv"

I can confirm it works and produces the expected output. Not sure if that works on Windows, too?
M\If not, maybe Pathlib has a clever way of handling paths with spaces cleverly and platform-indepenent.

@kvid
Copy link
Collaborator Author

kvid commented Jul 26, 2020

I can confirm it works and produces the expected output. Not sure if that works onWindows, too?

Thank's. I have earlier used the same method in Windows when the path contains spaces. It seems to be a portable method.

M\If not, maybe Pathlib has a clever way of handling paths with spaces cleverly and platform-indepenent.

I will have a look.

The best is perhaps to use a Python package for git to avoid os.system(), but maybe it's bettter to fix the important things first.

formatc1702 and others added 7 commits July 26, 2020 18:53
- Use `pathlib.Path` instead of `os.path`
- Fix order of files while building
- Consolidate code for building demos, examples and tutorial
- Change argument from `tutorials` to `tutorial` to remain consistent
- Add some indentation in console output for better readability
Add new actions:
- 'compare' action to compare generated files (except those
  generated by Graphviz) against the latest commit, and
- 'restore' action to restore generated files from the latest commit.

This is a squash rebase of these commits:
- p 9ad3e13 Reduce code duplication by moving common code into a generic function
- s d4feae6 Add action to restore generated files from git repository
- s 64f6507 Add action to compare generated files against git repository
- s 099c202 Simplify code
This make it possible to append '-g' or '--groups' followed by
space separated group names to any CLI action command, and the
set of generated files affected by the command will be limited
to the selected groups ('examples', 'tutorial', and 'demos').
Default is all groups. A simple help text is added for each of
the arguments (action and groups) to improve the autogenerated
CLI help output.

This is a squash rebase of these commits:
- p ec29076 Make all actions honor the optional argument -generate
- s e3ad11a Move open_file_append() outside the if to avoid re-open
- s ba4b900 Avoid including readme in all file groups
- s 1ca8bd1 Simplify code
- s a9e7337 Rename some variables to better reflect their contents and relations
- s 58a54b2 Move test to include readme inside collect_filenames() function
- s f2a0db0 Improve status output by adding group name
- s d3b299b Rename -generate option to -g/--groups and add argument help
By putting all value entries on separate lines with a trailing comma,
it becomes easier to read the diff when later inserting or deleting
the first or last value entry in any dict.
Now, all action functions are called with a group list as argument.
- Add double quotes around path string in `os.system()` call and
  status output to handle any spaces in the path.
- Split the `generated_extensions` list into the two lists
  `extensions_not_containing_graphviz_output` and
  `extensions_containing_graphviz_output` for readability.
@formatc1702
Copy link
Collaborator

formatc1702 commented Jul 27, 2020

Very nice, it works now!
The click implementation is not strictly necessary, so if you're happy with the result, I'm OK with merging it as-is.

@formatc1702 formatc1702 merged commit 99ca185 into wireviz:dev Jul 27, 2020
@kvid kvid mentioned this pull request Jul 27, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants