-
-
Notifications
You must be signed in to change notification settings - Fork 7
Try a different module/package structure #151
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
base: main
Are you sure you want to change the base?
Conversation
68c5b9e to
eb455f9
Compare
Codecov Report
@@ Coverage Diff @@
## main #151 +/- ##
==========================================
+ Coverage 94.78% 95.00% +0.22%
==========================================
Files 3 6 +3
Lines 728 761 +33
Branches 216 204 -12
==========================================
+ Hits 690 723 +33
Misses 14 14
Partials 24 24
Continue to review full report at Codecov.
|
|
I suspect the coverage change is due to changes in line numbers. |
|
Happy to keep rebasing/fixing conflicts so we can get @Th3nn3ss 's & @mr-c 's PRs in first 👋 |
ntachukwu
left a comment
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 don't fully understand every implementation in this PR but am very excited to see how efficiently it handles dependencies between the python files. I hope to have pythonic powers like this someday. Incredible work.
|
Will update this PR over the next days 💪 |
eb455f9 to
c10a152
Compare
|
Rebased, updated the code, and I think I got all the latest changes 🤓 😪 Going to stop for now, and review it in the morning. Then try to wrap it up updating anything broken in the CI. Or, if nothing broken, then just mark it as ready for review. |
31487f7 to
82252b7
Compare
82252b7 to
240439e
Compare
|
Done! |
It started over the week/holiday when I had a go at trying to write the conversion for WDL stdlib's
select_allfunction. It didn't work for other reasons, but I noticed that themain.pykeeps getting longer and longer.So I thought we could probably reduce it a little by starting to use more modules/packages.
After reading the
miniwdlcode, I thought we could have a structure similar to their structure, but also thinking about CWL.With that in mind, I thought about an
wdl2cwl.exprmodule with the WDL Expr statements (conditionals, arithmetic, etc). And then remove theifstatement from theWDL.Expr.Applyfor WDL Stdlib Functions, and move that code to a separate module as well.That resulted in
wdl2cwl.functions. Furthermore, instead of using anif/elif's/elsestatement, we can simply check if the module has a function with the same name (coding by convention) and use it if so, otherwise raise an error as before.While that would reduce the code, not sure if that's the best/right call for now. So happy to get any feedback. I almost finished this change, but ran out of ☕ (head was literally bobbing while I was writing the code 😴 , will review in the morning 🛌 )
Cheers
-Bruno