Extended build examples #118
Conversation
|
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 😎 |
|
There might be a minor issue about the readme beeing part of all three groups of generated files for all actions except |
|
I'll wait until you fix it, it's not urgent. |
kvid
force-pushed
the
extended-build-examples
branch
from
6a2b197
to
1ca8bd1
Compare
|
Should I rename the |
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 |
kvid
force-pushed
the
extended-build-examples
branch
from
78b9d36
to
d3b299b
Compare
|
Honestly, with all your hard work, it probably makes sense to squash and merge all your commits directly into
Just let me know when you are ready, then I can test and do as described, if you think it's OK. |
|
@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 |
|
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 |
|
I can also edit this PR to point to |
|
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.
What will then happen to the commits on your |
|
Feel free to rebase onto dev, force-push and restructure any way you want, and I will preserve your commits :) |
kvid
force-pushed
the
extended-build-examples
branch
3 times, most recently
from
9857eee
to
67f5e20
Compare
|
@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 |
|
I guess it doesn't hurt to keep the .zip file, feel free to put it back in. |
|
@formatc1702 wrote:
Then this PR is ready for merge-in if it passes your review. The commit 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. |
|
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) The |
src/wireviz/build_examples.py
Outdated
|
|
||
| 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'] |
There was a problem hiding this comment.
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!
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
| 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)') |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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()There was a problem hiding this comment.
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.
Thank's for telling me immediately. I guess double quotes around the filenames should help. Can you try this command manually in MacOS? |
I can confirm it works and produces the expected output. Not sure if that works on Windows, too? |
Thank's. I have earlier used the same method in Windows when the path contains spaces. It seems to be a portable method.
I will have a look. The best is perhaps to use a Python package for git to avoid |
- 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.
kvid
force-pushed
the
extended-build-examples
branch
from
8d86c65
to
39487ca
Compare
|
Very nice, it works now! |
This PR
builds on top of #110 andadd some new features:compareaction to compare generated files (except those generated by Graphviz unless using-c) against git repository.restoreaction to restore generated files from git repository.-g. This make it possible to select the group of files to use for any action by following the action argument with-gand then the space separated group names.Edit: It should not break the existing
buildandcleanactions, 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:
-generateis renamed to-g/--groupsand some argument help is added.{description} {groupkey} in {path}e.g.Building demos in /path/WireViz/examples.Usage:
python build_examples.pyto build generated files in all groups.python build_examples.py compareto compare generated files in all groups against the last commit.python build_examples.py cleanto delete generated files in all groups.python build_examples.py restoreto restore generated files in all groups from the last commit.-cor--compare-graphviz-outputto thecomparecommand above to also compare the Graphviz output (default: False).-gor--groupsfollowed 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.python build_examples.py -hor with--helpto 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
devto 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.pyfollowed bypython build_examples.py compareas 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 usepython build_examples.py restorejust before committing to avoid unwanted changes from e.g. a different Graphviz version. Hopefully, others also find this useful.