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

Add warning in API concerning properties containing model variables #557

Merged
merged 2 commits into from
Jun 1, 2024

Conversation

AlexBork
Copy link
Contributor

The parseProperties function taking only a string as input is not able to parse properties containing model variables as these are not registered with the created ExpressionManager.
This is expected behaviour, however, the resulting error message of the form

ERROR (SpiritErrorHandler.h:27): Parsing error at 1:2:  expecting <basic path formula>, here:
	X (var = 0)]
	 ^

does not point to the actual problem, making debugging the issue difficult, in particular for inexperienced users.
This PR adds a warning to the function that makes the issue more apparent.

Thanks to @SvStein for bringing this to my attention.

@AlexBork
Copy link
Contributor Author

AlexBork commented May 28, 2024

If you think the warning should only come up when in DEBUG mode, please let me know.

I also considered making the parser error message more meaningful, though that seems to require a lot more changes for a use cases I don't expect to come up too often.

@volkm
Copy link
Contributor

volkm commented May 29, 2024

The current version would always output a warning. I imagine this could be quite confusing for users who input a string without model parameters, because they have to ignore the warning.
Would it be possible to provide the warning/error somewhere in the Spirit parser such that the output only occurs when really needed? (From your comment I guess that this would be too work though?

@sjunges
Copy link
Contributor

sjunges commented May 29, 2024

Isnt that exactly iff the error is thrown?

@AlexBork
Copy link
Contributor Author

The error can also occur when there is a different issue with the input, so it's not an iff.
I'd like to give a hint if parsing fails in the specific context of the API call as someone who is not fully aware of how the parser works (i.e. someone like me) would assume that giving a syntactically valid formula should parse.
So the error message/warning/hint should not come up in all contexts as it might also be confusing.

Would adding an error handler to the API fucntion be an option? That way, we can give the additional hint only if the specific error occurs in the context of the API call.

@volkm
Copy link
Contributor

volkm commented May 29, 2024

Does the Spirit parser throw an exception which we could catch in the API call and then output the additional information?

@volkm
Copy link
Contributor

volkm commented May 29, 2024

The updated version looks good to me. Thanks for improving the usability.

@sjunges
Copy link
Contributor

sjunges commented Jun 1, 2024

@linusheck this was what we discussed last week!

@sjunges sjunges merged commit 373de5e into moves-rwth:master Jun 1, 2024
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants