Fix #45: The optional '::' in a module procedure declaration was not #46
Conversation
accepted. Added test case for usage of '::", and also test cases for module procedure declaration without the (optional) 'module'.
arporter
left a comment
arporter
left a comment
There was a problem hiding this comment.
I think \s should be used in the regex rather than " " but apart from that changes are good. Relevant test is extended and still covers the only changed line.
Only significant issue is that we are trying to improve the code base and therefore we apply our coding rules to any changed routines. This means that the test routine needs a doc string and some other minor changes.
src/fparser/statements.py
Outdated
| [ MODULE ] PROCEDURE [::] <procedure-name-list> | ||
| """ | ||
| match = re.compile(r'(module\s*|)procedure\b',re.I).match | ||
| match = re.compile(r'(module\s*|)procedure\b *(::)?',re.I).match |
There was a problem hiding this comment.
Should that be "\s*" rather than " *"?
src/fparser/tests/test_parser.py
Outdated
| 'module procedure :: a '), 'MODULE PROCEDURE a') | ||
| assert_equal(parse(ModuleProcedure,\ | ||
| 'module procedure :: a , b'), 'MODULE PROCEDURE a, b') | ||
| assert_equal(parse(ModuleProcedure,\ |
There was a problem hiding this comment.
Extend existing test to check that fparser is happy with/without the "module" prefix and/or the "::" separator. Great.
There was a problem hiding this comment.
As per the review guidelines (https://github.com/stfc/fparser/wiki/CodeReview), please could you update this routine to be pep8 and pylint compliant (i.e., two blank lines before and after it, a docstring etc.). Also, we're trying to move away from using nose so please could you replace all uses of "assert_equal" with just "assert" in this routine.
|
OK, I did most of it (see also pull request coding style - which might have some minor conflicts with this one). |
|
Wow, you didn't need to do the whole file but thanks very much! Changes in this PR are now all good. |
3 participants
accepted. Added test case for usage of '::", and also test cases
for module procedure declaration without the (optional) 'module'.