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

Unify arguments "converter" and "subconverters" #28

Open
wkirgsn opened this issue Apr 29, 2020 · 2 comments
Open

Unify arguments "converter" and "subconverters" #28

wkirgsn opened this issue Apr 29, 2020 · 2 comments

Comments

@wkirgsn
Copy link
Member

wkirgsn commented Apr 29, 2020

in case of multiple converters, both arguments "converter" and "subconverters" have to be set.
It would be more user friendly to just go with one argument "converter", which can be either a string or a list of strings, in case of multiple converters.

@wkirgsn wkirgsn changed the title unify arguments "converter" and "subconverters" Unify arguments "converter" and "subconverters" Apr 29, 2020
@atra94
Copy link
Collaborator

atra94 commented May 11, 2020

I agree with you, that it would be more user friendly to reduce it to one argument. But at the moment, I only see one simple solution to this problem.

You could just write an if-else clause in the SCMLSystem init-method, which checks the type of the converter-argument. But this would introduce a dependency between the SCMLSystem and a special converter class that I would avoid, because it would complicate the use of a different implementation that merges two converters.

Furthermore we would end up writing spaghetti-code, if we introduce such dependencies everywhere.

If such requirements of using multiple different converters will appear moreoften in the future, a redesign of the SCMLSystem to be able to handle multipleConverters inherently might be useful, because it was designed for the DC and Synchronous Motors. This solution would require much more effort and should consider a bunch of future requirements.

So finally, I would appreciate an elegant solution that handles this issue, but I would avoid additional dependencies between classes.

@wallscheid
Copy link
Collaborator

Suitable follow-up after #48 is implemented.

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

No branches or pull requests

3 participants