-
Notifications
You must be signed in to change notification settings - Fork 56
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
Tool restructuring #311
Tool restructuring #311
Conversation
Like the idea, I guess @adobekan would agree, as we have discussed a similar idea. One main advantage would also be that you can "pip install " vss-tools, import sutff and create your custom exporter without touching COVESA upstream code. Today users are basically "forced" to fork, and I have seen such internals forks flying/rotting around. Resistance to pick up new features upstream would likely be lower, if you just need to upgrade a dependency |
vspec2x_abstract.py
Outdated
def main(self, arguments): | ||
initLogging() | ||
|
||
parser.add_argument('-I', '--include-dir', action='append', metavar='dir', type=str, default=[], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be good to change logic, that somehthing like vspec2json "owns" the parser for command line arguments and can include/instanctiate vpsec2x with a feature array (do I know instances/structs/uuid/extended attributes), and then vspec2x would "add" generic options , and leave options out that a tool does not support anyway
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be quite easy to do some changes of the current solution. Today we have if , if looking for instance at vss2json.py two methods:
def feature_supported(feature_name: str)
def add_arguments(parser: argparse.ArgumentParser):
Where add_arguments
is for adding exporter-specific arguments, and feature_supported
to answer yes/no on features like "no expand" (for struct we have a different mechanism), but I agree that passing in some form of config array would be a nicer solution, as we then could exclude non-supported arguments already when you do -h
.
Meeting notes:
|
6db9454
to
fc73044
Compare
079ab4d
to
916cb28
Compare
58b17e9
to
638fd5b
Compare
Ready for review new. |
ff3b513
to
4f65f31
Compare
@adobekan - could you take a look and check if you have any objections to the content of this PR. If not I will merge it. |
Meeting notes:
|
MoM: Ok to merge, after the static id PR has been merged |
Refactoring tool structure so that there is not a single entrypoint (vspec2x.py) but rather individual ones. Makes it easier to add custom generators. Signed-off-by: Erik Jaegervall <[email protected]>
4f65f31
to
773ad87
Compare
This is an idea I have for restructuring of vss-tools. Ass of today we have vspec2x.py as main entry point. It explicitly lists all exporters and import all of them as well. This has some drawbacks:
vspec2x.py
An alternative solution would be to instead have
vspec2x
as an abstract tool that require an exporter as input. That would make it easier to create you own proprietary tool/exporter, you do not need to modifyvspec2x.py
, instead you could just create you own exporter and give it as an argument when calling vspec2x. This PR shows how that could look like for vspec2json. The filevspec2x_abstract.py
is intended to replacevspec2x.py
If we go this path we could as a second step consider refactoring our vss-tools PyPI package so that exporters with "heavy" dependencies (big size, or dependencies that only are supported on a limited set of platforms, or dependencies to less user friendly licenses) are put in a separate PyPI package. A possible example could be graphql. We could also possibly add generic exporter functionality/hooks so that exporters can add or control additional semantic checks or processing steps if needed. A typical example could be the recent discussion to expand signal tree based on allowed units. Maybe that is a feature that is possible or even mandatory for one exporter, but totally useless for others.