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

LIU-251 Automatically Parse App Args to Params #159

Merged
merged 7 commits into from
May 24, 2022
Merged

Conversation

calgray
Copy link
Collaborator

@calgray calgray commented May 20, 2022

This change combines drop app params with dlg_x_param() definitions. Params can now be either defined as component params or app params and AbstractDrop will give priority loading to component params then app params. This was done for backwards compatibility as well as supporting tests that don't write to 'applicationArgs'.

Comment on lines 397 to 411
def get_param_value(attr_name, default_value):
has_component_param = attr_name in kwargs
has_app_param = hasattr(self, 'parameters') \
and 'applicationArgs' in self.parameters \
and attr_name in self.parameters['applicationArgs']

if has_component_param and has_app_param:
logger.warning(f"Drop has both component and app param {attr_name}. Using component param.")
if has_component_param:
param = kwargs.get(attr_name)
elif has_app_param:
param = self.parameters['applicationArgs'].get(attr_name).value
else:
param = default_value
return param
Copy link
Collaborator Author

@calgray calgray May 20, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This block controls the priority between component params and app params.
There is some complication where I can't find a reason to load applicationArgs['name'].default_value since I'd expect eagle to populate an appropriate applicationArgs['name'].value, if the key is missing the app default is used.

@coveralls
Copy link

coveralls commented May 20, 2022

Coverage Status

Coverage decreased (-0.06%) to 80.926% when pulling ee74b97 on LIU-251-app-params into 71b95f5 on master.

@calgray calgray requested a review from awicenec May 20, 2022 07:34
@calgray calgray changed the title LIU-251 Automatically Parse App Params LIU-251 Automatically Parse App Args to Params May 24, 2022
Copy link
Collaborator

@james-strauss-uwa james-strauss-uwa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, a great improvement.

@calgray
Copy link
Collaborator Author

calgray commented May 24, 2022

just noticed the cause of test change after realizing drop._getArg(kwargs,..) is actually more like drop._popArg(kwargs,..). I should be able refactor without this sideaffect.

@calgray calgray merged commit 046c159 into master May 24, 2022
@calgray calgray mentioned this pull request May 27, 2022
@awicenec awicenec deleted the LIU-251-app-params branch May 9, 2023 12:56
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

Successfully merging this pull request may close these issues.

3 participants