-
-
Notifications
You must be signed in to change notification settings - Fork 8
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
Try a different module/package structure #151
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 👋 |
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_all
function. It didn't work for other reasons, but I noticed that themain.py
keeps getting longer and longer.So I thought we could probably reduce it a little by starting to use more modules/packages.
After reading the
miniwdl
code, 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.expr
module with the WDL Expr statements (conditionals, arithmetic, etc). And then remove theif
statement from theWDL.Expr.Apply
for 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/else
statement, 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