Conversation
- Do not instantiate full instance of loader when displaying help - Process all known arguments
|
@twiggler I created a draft PR of this branch to make looking and reviewing it easier |
| # Instantiate the loader for the first target path (used for all targets in this run). | ||
| # For URI-based targets the path component and parsed_path are extracted by infer_loader. | ||
| if loader_cls is not None and args.targets: | ||
| if args.loader: | ||
| first_spec = args.targets[0] | ||
| adjusted_path, parsed_path = parse_path_uri(first_spec) | ||
| load_path = adjusted_path if parsed_path is not None else pathlib.Path(first_spec) | ||
| else: | ||
| result = infer_loader(args.targets[0]) | ||
| if result is not None: | ||
| _, load_path, parsed_path = result | ||
| else: | ||
| load_path = args.targets[0] | ||
| parsed_path = None | ||
|
|
||
| try: | ||
| loader_instance = loader_cls(load_path, parsed_path=parsed_path, loader_kwargs=loader_kwargs) | ||
| except Exception as e: | ||
| parser.error(f"Failed to instantiate loader '{loader_cls.__name__}': {e}") |
There was a problem hiding this comment.
While the loader logic is extracted, there is now a restriction that you can't use target-query on targets that require different loaders.
For example:
target-query -f services /path/to/file.vmx /path/to/acquire.tar
Requires two different loaders and only the first one will properly execute services for file.vmx
This is fine in the case that we want to see the help with the loader, but would be a regression when using it with plugins across targets.
|
|
||
| try: | ||
| for target in open_targets(args): | ||
| for target in open_targets(args, loader_instance=loader_instance): |
There was a problem hiding this comment.
within the context of the previous comment, wouldn't it be wise to determine the loader_class inside open_target?
| try: | ||
| yield from target.open_children(apply=apply) | ||
| except Exception as e: | ||
| get_target_logger(load_spec).error("Failed to load child target from %s", target, exc_info=e) |
There was a problem hiding this comment.
technically you can use target.log.error here no? As target is already instantiated.
| ) | ||
| if direct: | ||
| targets: Iterable[Target] = [Target.open_direct(args.targets)] | ||
| elif loader_instance is not None: |
There was a problem hiding this comment.
with the current code loader_instance will never be None wouldn't it make sense to only do this if -L <> was specified? So args.loader was declared or something?
2 participants
No description provided.