Add list_dependencies verb #133
Conversation
|
Hi @rolling-robot, I tried this code and it just works 🎉 👍 Few comments on this PR:
Minor style nipticks (We can fix them for you once this PR has been approved, it's more to let you know the few style gotchas that are not enforced by our linters):
|
|
Thanks for the quick iteration! I just noticed that the list is not filtered before being printed. Do you think you could remove the duplicates before displaying the dependency list ? Nitpick: It looks like the travis job failed for a style thing (missing space after a |
| type=argparse_existing_dir, | ||
| default=os.curdir, | ||
| help='Base paths to recursively crawl for packages', | ||
| ) |
There was a problem hiding this comment.
Having an optional positional argument changes the semantic of the first passed argument based on if a second argument is passed or not. I think that is counter intuitive and I would suggest using a non-positional argument for the base path instead.
| deps = [] | ||
| packages = find_packages(options.basepath) | ||
| # show all dependencies if no options are given | ||
| if(not (options.build_deps or options.test_deps or options.exec_deps)): |
There was a problem hiding this comment.
Please remove the unnecessary outer parenthesis.
| package.buildtool_export_depends + | ||
| package.build_depends + | ||
| package.buildtool_depends + | ||
| package.doc_depends |
There was a problem hiding this comment.
I wouldn't consider doc_depends a build dependency. Maybe it would be best to add a separate option to list doc dependencies and remove them from this set.
| for line in sorted(set(map(lambda dep: dep.name, deps))): | ||
| print(line) | ||
| return | ||
| print('No package with name {!r} found'.format(options.package)) |
There was a problem hiding this comment.
Error messages should go to stderr.
completion/ament-completion.bash
Outdated
| COMPREPLY=($(compgen -W "$(ament list_packages --names-only)" -- ${cur})) | ||
| else | ||
| COMPREPLY=($(compgen -W "--build-deps --exec-deps --test-deps $(ament list_packages --names-only)" -- ${cur})) | ||
| fi |
There was a problem hiding this comment.
The completion doesn't consider the passed basepath atm.
| deps = [] | ||
| packages = find_packages(options.basepath) | ||
| # show all dependencies if no options are given | ||
| if not (options.build_deps or options.doc_depends or options.exec_deps or options.test_deps): |
There was a problem hiding this comment.
This should be doc_deps to match the argument defined in the parser, same thing line 63
There was a problem hiding this comment.
Yeah, silly typo.
| for line in sorted(set(map(lambda dep: dep.name, deps))): | ||
| print(line) | ||
| return | ||
| print('No package with name {!r} found'.format(options.package), sys.stderr) |
There was a problem hiding this comment.
this still prints to standard output, I think you want file=sys.stderr here
|
Hi @rolling-robot, Sorry for letting this slide. Except one or two comments it looks good to me! Thanks for iterating on this @dirk-thomas any more comments on this ? |
| if options.build_deps: | ||
| deps.extend( | ||
| package.build_export_depends + | ||
| package.buildtool_export_depends + |
There was a problem hiding this comment.
The two build export dependencies are not necessary for building but are made available to downstream packages. Therefore I would recommend to move them into the exec_deps block.
(Sorry I didn't notice this is the first round of review.)
| ) | ||
| if options.doc_deps: | ||
| deps.extend(package.doc_depends) | ||
| if options.exec_deps: |
There was a problem hiding this comment.
Based on the comment above I would recommend to change this option to run_deps (which will include the exec depends as well as both build export depends).
|
thank you both for iterating on this. I'll go ahead and merge this. |
|
Thanks everyone! |
|
@rolling-robot Thank you! |
Hello,
This commit is intended to fix #113, however I have found another bug related to command line options. See osrf/osrf_pycommon#33