-
Notifications
You must be signed in to change notification settings - Fork 12
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
String-based expression substitution and moar expression tests! #366
Conversation
Documentation for this branch can be viewed at https://sites.ecmwf.int/docs/loki/366/index.html |
e226867
to
9d38dec
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #366 +/- ##
=======================================
Coverage 95.41% 95.42%
=======================================
Files 178 180 +2
Lines 37312 37387 +75
=======================================
+ Hits 35600 35675 +75
Misses 1712 1712
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
9d38dec
to
8b96fd6
Compare
8b96fd6
to
eefed4d
Compare
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.
The SubstituteStringExpressions
utility is a really nice one! Obviously, loads of ways how someone can use this in dangerous ways, so we need to watch out for this.
The test clean-up and extended explicit coverage is much appreciated!
Good to go from me!
loki/expression/tests/test_parser.py
Outdated
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've not gone through this in detail but I'm trusting this is largely just a source move.
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.
Yes, it is. I tried to be careful here. 😉
""" | ||
fcode = """ | ||
module test_mod | ||
use external_mod, only: rick |
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.
use external_mod, only: rick | |
use external_mod, only: rick | |
implicit none |
missing also from a few other places in this file, 'cause, wouldn't want to risk Rick letting me down due to implicit typing after all.
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.
Rick would never tell a lie and hurt you. Fixed now 😉
This change separates some of the
loki.expression
tests into separate sub-packages and adds a newSubstituteStringExpressions
utility that builds on the recently addedparse_expr
utility for expression substitution.The test separation is meant to reduce the size of the general expression test, which is growing slightly out of proportions. I've also added some explicit test of the much used
SubstituteExpressions
transformer.Steps in detail:
loki.expression.test.test_expr_visitors
sub-package with some existing tests and new tests for expression substitution; this includes an added test forFindVariables
which was previously only covered implicitly (it seems)loki.expression.tests.test_parser
SubstituteStringExpressions
transformer that works pretty much as theSubstituteExpressions
but usesparse_expr
to use string mapping as input.