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 247 #145

Merged
merged 16 commits into from
May 8, 2022
Merged

Liu 247 #145

merged 16 commits into from
May 8, 2022

Conversation

awicenec
Copy link
Contributor

@awicenec awicenec commented May 5, 2022

Main objective was to support *args and **kwargs for PyfuncApps. In addition to that we are now also supporting positional only args and kwargs only. Thus we are covering all Python cases. Additional changes:

  • Fixed the issue with the Travis version of doxygen wrongly extracting information from the docstring, if that contained highlighting markup. That messed up the hover help text of a number of components, including the updated PyfuncApp. The fix involved moving the Travis tests to Ubuntu Focal. Also cleaned up the default palettes extracted by xml2palette and enabled the extraction of a few more of the existing apps.
  • added a dummy PythonApp to simple.py to trigger the extraction of the associated palette documentation. The PythonApp palette component only existed as a hard-coded component in EAGLE.
  • Aligned the EnvironmentApp category with EAGLE, where this was called EnvironmentVariables.

@awicenec awicenec requested a review from rtobar May 5, 2022 12:48
@coveralls
Copy link

coveralls commented May 5, 2022

Coverage Status

Coverage increased (+0.8%) to 81.193% when pulling a310776 on liu-247 into 3068a2f on master.

Copy link
Contributor

@rtobar rtobar left a comment

Choose a reason for hiding this comment

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

I just left some minor comments, but those are not too important.

What I find more substantial is that most of the new code in pyfunc.py is not tested:
https://coveralls.io/builds/48860216/source?filename=daliuge-engine%2Fdlg%2Fapps%2Fpyfunc.py. I'm guessing some manual tests were done, but it'd be great to see unit tests for these new bits, as otherwise we'll quickly derail when new changes come in.

daliuge-engine/dlg/apps/pyfunc.py Show resolved Hide resolved
@@ -63,6 +63,36 @@ class NullBarrierApp(BarrierAppDROP):
def run(self):
pass

##
Copy link
Contributor

Choose a reason for hiding this comment

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

It's not crystal clear what the idea of this new PythonApp is, given it's almost identical to SleepApp except for the name and for not having the sleepTime parameter. If it's here to stay let's at least add a test for it just to ensure it's actually correct (it does look correct though).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As mentioned in the original comment for this PR the PythonApp is required to enable people to develop a graph and a palette from scratch in EAGLE. It always existed for that reason, but was part of the hard-coded palette in EAGLE. Since a PythonApp is in some way such a fundamental component in DALiuGE, there was no class with that name in the code and thus this was the only normal component missing from the EAGLE hard-coded palette. This class only exists to enable the automatic creation of the associated component in the Raw Component palette. Since the appclass parameter in the EAGLE doxygen section is actually empty, the code will in fact never be called (although it is functional). Thus we could go a step further and just put a 'pass' into the code body.

@awicenec awicenec merged commit db148ca into master May 8, 2022
@awicenec awicenec deleted the liu-247 branch May 8, 2022 13:00
awicenec added a commit that referenced this pull request May 19, 2022
pritchardn pushed a commit that referenced this pull request May 20, 2022
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