Codecov Report

Merging #239 into master will increase coverage by 0.16%.
The diff coverage is 99.51%.

@@            Coverage Diff             @@
##           master     #239      +/-   ##
==========================================
+ Coverage   90.24%   90.41%   +0.16%     
==========================================
  Files          34       35       +1     
  Lines       12150    12349     +199     
==========================================
+ Hits        10965    11165     +200     
+ Misses       1185     1184       -1

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 866d0c7...985433a. Read the comment docs.

OK, apparently the section symbol used in the draft isn't ASCII... removed it from comments and docstring

Thanks a lot for your review and the suggested changes!
I'll work through as much as I can now and let you know once I think it's worth having another look

@rupertford I tried to address all of your requested changes.

I have marked most of your comments as resolved after having worked on that bit simply to keep track of what remains to be done (Not sure if this is just a personal setting or if they appear like that for you as well). It may not necessarily mean that the raised point is resolved to your satisfaction ;-)

When you find time I'd be glad if you could have another look and come back with some further remarks.

Thanks @reuterbal. I've changed the status to ready-for-review and will review when I next get the chance. For the record, our policy is that the reviewer marks things resolved :-). We would be happy to give you access to the repo if you would prefer to work here rather than on a fork.

Sorry, I un-resolved them again...

I would not mind repository access but it's also not a problem to work on a fork. It doesn't really change much for me. Any changes that I have to make I will send your way as a PR anyways and there will be an issue about that beforehand (unless it's something very tiny like #241).
I have an additional branch loki_integration on my fork which I only use to merge branches that are not yet in fparser's master but are necessary for my downstream work. This makes it easier for me in the CI testing.

Thanks for looking through it again. Code duplication is a fair point that I did not think about since most of the matching was already implemented by @martin-schlipf and I changed only a few bits.
One concern would probably be the fact that preprocessor directives are case-sensitive (I think) and everything in utils.py is not. The question is, whether to make case sensitive matching an optional feature in WORDClsBase etc. or a very similar base class should be added?

Looking through the relevant matching routines and leaving case sensitivity aside, I think the following could be done:

• Match the # explicitly and use one of the classes from utils.py to match the remainder of the line
• Cpp_If_Stmt, Cpp_Elif_Stmt: Introduce a new object for the right hand side (if it's not an if(n)def) instead of keeping it as a string and use WORDClsBase to do the matching (similar to, e.g., Intrinsic_Type_Spec).
• Cpp_Else_Stmt, Cpp_Endif_Stmt: Use WORDClsBase without cls
• Cpp_Undef_Stmt: Use WORDClsBase with
• Cpp_Include_Stmt: Keep as is (similar to Include_Stmt)
• Cpp_Line_Stmt: Use WORDClsBase with a new right hand side class (either a new one matching closely the standard or a rather loose matching using the one for if/elif)
• Cpp_Macro_Stmt: This one could prove a bit more difficult to get the handling of white spaces and the different meanings of those right. Could maybe work with a clever regex and STRINGBase. Otherwise I would just keep it as is.
• Cpp_Macro_Identifier_List: Put the regex into pattern_tools.py and use StringBase. I also noticed that the regex is not yet fully correct as it allows for the variadic argument dots in any position (not just the last).
• Cpp_Error_Stmt, Cpp_Warning_Stmt: Use WORDClsBase with a new cls.
• Cpp_Null_Stmt: Keep as is.

Thus, in principal it should be possible but the question is how to take care of case sensitivity.

Yes, good point about case insensitive matching, I forgot about that.

The simplest thing would probably be to add an an ignore_case=True to the match method in WORDClsBase() and then set that to False in the new CPP matches. I think the only code change required would be to if line[:len(keyword)].upper() != keyword.upper():. I would accept a new issue for adding tests to WORDClsBase rather than making you add new tests for it (unless you want to).

A nicer way would be to do a case sensitive match and then subclass this for a case insensitive match and use WordClsBase for case sensitive and WORDClsBase for case insensitive, following the StringBase and STRINGBase naming convention. However that would probably be a separate PR.

If it's OK I would go with the ignore_case=True option to WORDClsBase for now and opening an issue to remove this and implement the suggested hierarchy instead -- or at least adding the relevant tests. I might find time inbetween to do that but I would like to get on with the other issues first.

Fine by me.

No need to apologize, really. I appreciate the scrutiny.
This should address all of the remarks - sorry from my side for repeating mistakes that were mentioned before.
Code duplication in the tests should be significantly reduced and I hope this is now on an acceptable level.
Codecov reporting is somewhat off for me at the moment. It gave <100% for C99Preprocessor after I added the check for CppDirective, I fixed this by combining return None cases and now the reporting is screwed up. However, looking at the details on codecov.io doesn't give any red line. Maybe it'll repair the message itself later?

Many thanks, happy to hear that :)
Just for clarity: I should add my name in the relevant places in doc/conf.pyand doc/license.rst? What ruleset do you follow regarding order of names? (Out of instinct I would have gone for 3rd place?)

Many thanks, happy to hear that :)
Just for clarity: I should add my name in the relevant places in doc/conf.pyand doc/license.rst? What ruleset do you follow regarding order of names? (Out of instinct I would have gone for 3rd place?)

I've just talked to @arporter about this. We agree that you should go 3rd if you are OK with that. We don't currently have an official policy, it was just the order in which people contributed to the documentation (in time) + Pearu, so it is at least consistent with that informal policy. We agree there should be an official policy but suggest we park that decision for the time being.

And, yes, the files you mention are the correct ones. If you wouldn't mind updating the copyright date to 2020 as well that would be great (but the PR will still be approved if you don't :-)).

I have moved the fixture and updated the author list (changed copyright dates wherever I've seen some). I don't care much about the order but wanted to avoid tipping on any toes.

Many thanks again for taking the time to work through this big change. This is a very crucial feature for our downstream tools and I'm very happy that it makes its way into master.

Just for clarification: Is it correct, that only C preprocessor directives are supported? I'm currently stumbling over CoCo directives starting with ?? macroname..., but it seems that those can't be handled with this PR out of the box, right?

Just for clarification: Is it correct, that only C preprocessor directives are supported? I'm currently stumbling over CoCo directives starting with ?? macroname..., but it seems that those can't be handled with this PR out of the box, right?

Yes that's right . This PR added support for cpp directives. Do you need to reason about the CoCo directives or would it be OK if they were removed?

Yes that's right . This PR added support for cpp directives. Do you need to reason about the CoCo directives or would it be OK if they were removed?

No, for my current task it would be sufficient to ignore them.

At the moment the culprit is this one:

% python -m ring_include.py
Traceback (most recent call last):
  File "/opt/local/Library/Frameworks/Python.framework/Versions/3.8/lib/python3.8/site-packages/fparser/two/Fortran2003.py", line 237, in __new__
    return Base.__new__(cls, string)
  File "/opt/local/Library/Frameworks/Python.framework/Versions/3.8/lib/python3.8/site-packages/fparser/two/utils.py", line 407, in __new__
    raise NoMatchError(errmsg)
fparser.two.utils.NoMatchError: at line 9
>>>?? include 'header/lbm_macros.inc'

But I fear that this won't be as straight forward as there are lines starting with ?? like above, but also macros within code lines like:

value = ?MACRONAME?(here, come, some, arguments)

However, I never came across one of those yet, so I don't know whether they'll also cause a NoMatchError.

No, for my current task it would be sufficient to ignore them. But I fear that this won't be as straight forward as there are lines starting with ?? but also macros within code lines like:

value = ?MACRONAME?(here, come, some, arguments)

Ah. Yes, that is a problem. Presumably you are not wanting/able to pre-process the code first. I don't know much about CoCo but pre-processor syntax and Fortran syntax are not typically designed to be mixed. We could follow the approach taken by the cpp support but it would have the same restriction that the code without the directives was valid Fortran (which would include, not allowing inline macro code unless the macros always aligned with Fortran tokens - then we could potentially extend the associated rule).

Well, I want to avoid operating on intermediate files. But I also want to create a working script, thus I'll go for the preprocessed files. Trying to get fparser to be able to cope with CoCo directives would take considerable effort, I presume. Nevertheless, thanks for your fast response :)

No problem. Getting fparser to cope with full line CoCo directives should be relatively simple, so if that would be useful we could look at adding that. Adding understanding about the Coco syntax of full line directives would be feasible, but a reasonable amount of effort, but it sounds like you don't need this. Adding support for inline macros would be considerable effort I suspect and might only work in certain circumstances.

Exactly, for my current problem I don't need to understand CoCo syntax. Additionally, we are making plans to switch to Fypp (in the not so near future), which would introduce it's own specialties. Thanks for clarifications and support. :)