Skip to content
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

Adding includeFunc in controlDict crashes FoamFile serialization #314

Open
sturmk opened this issue Jan 7, 2025 · 9 comments · May be fixed by #315
Open

Adding includeFunc in controlDict crashes FoamFile serialization #314

sturmk opened this issue Jan 7, 2025 · 9 comments · May be fixed by #315

Comments

@sturmk
Copy link

sturmk commented Jan 7, 2025

I'm trying to add a FOAM includeFunction in the system/controlDict:

import foamlib

controlDict = foamlib.FoamFile("./system/controlDict")
controlDict["functions"]["#includeFunc"] = "funcName"

this crashes my Python script and throws a lengthy pyparsing ParseException upon file serialization.

@gerlero
Copy link
Owner

gerlero commented Jan 7, 2025

@sturmk I'm able to reproduce the issue. To be fair, there's currently no support for directives starting with #include (they should get ignored as if they were just comments, but in this case it ends up breaking the parse).

May I ask, how would you expect your code snippet to change the file?

functions
{
    #includeFunc funcName;
}

Is that it? If so, I believe I could make a special case for #includeFunc so that this works...

@sturmk
Copy link
Author

sturmk commented Jan 7, 2025

@gerlero Indeed, #includeFunc is an edge-case.
It is also special, in that it should NOT end the line with a semi-colon. So I'm not quite sure if there's more effort involved to make it work.

@gerlero
Copy link
Owner

gerlero commented Jan 7, 2025

What do you think of making a special case for all #-directives, so that they behave as special leaf entries that don't end at a semicolon (instead, they go until a \n)? Not really asking for you to think this through... more of thinking out loud. I think that could be a more general solution for this one.

@sturmk
Copy link
Author

sturmk commented Jan 8, 2025

Would this mean that a user then has to enter the directive like this:
controlDict["functions"]["#includeFunc"] = "funcName\n" ?
Not sure I understood your suggestion for the line break. I'd probably prefer a solution without an explicit \n

@gerlero
Copy link
Owner

gerlero commented Jan 8, 2025

@sturmk Actually, I was thinking the \n could be added automatically.

My idea is to make a sort of special case for keywords starting with #, so that as in your example:

controlDict["functions"]["#includeFunc"] = "funcName"

directly maps to:

functions
{
    #includeFunc funcName
}

(with no semicolon at the at the end of funcName)

and vice versa.

That ideally should handle a lot of the #-stuff (notably except #codeStream, which works different and is more complex to parse).

Just as a note to myself, this would be a (small) breaking change given that #-directives are currently ignored by the parser. Not a problem though since (assuming it could be made work) it would be an improvement.

@sturmk
Copy link
Author

sturmk commented Jan 8, 2025

@gerlero perfect!

@gerlero gerlero linked a pull request Jan 8, 2025 that will close this issue
@gerlero
Copy link
Owner

gerlero commented Jan 8, 2025

I'm trying out the change at #315, where it seems to be working.

I need to ask you, though: how bad would you need this fix to be released right now?

I ask because this repo is now under active review for JOSS at openjournals/joss-reviews#7633. So, I'd prefer to delay making any (potentially breaking) releases right now as much as possible; yet I would still consider publishing a new version now it now if a user (like you) says they'd use the new functionality now.

@sturmk
Copy link
Author

sturmk commented Jan 9, 2025

@gerlero thank you very much! I applied the patch to my local foamlib installation; no need for a release from my side for now.

@gerlero
Copy link
Owner

gerlero commented Jan 9, 2025

@sturmk Thanks! I'll then make sure to merge the changes for the next release. Let me know if something still doesn't work.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants