aiplan4eu / unified-planning

The AIPlan4EU Unified Planning Library
Apache License 2.0
181 stars 39 forks source link

Fix ANML parser on MAJSP domain #604

Closed arbimo closed 3 months ago

arbimo commented 4 months ago

This PR identifies a parser problem on an existing ANML domain.

The second commit adds a quick and dirty fix for it. However I have no knowledge of how it is actually supposed to work (and did not really dig into it). Hence it is likely the case that the problem is deeper that what is handled by the fix, which I really obtained by "randomly" changing things until it stops crashing...

@Framba-Luca could you have a look at it and see if it should be improved/generalized?

codecov[bot] commented 4 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 85.29%. Comparing base (ee56971) to head (34e3070).

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #604 +/- ## ========================================== + Coverage 85.28% 85.29% +0.01% ========================================== Files 202 202 Lines 27245 27251 +6 ========================================== + Hits 23237 23245 +8 + Misses 4008 4006 -2 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

Framba-Luca commented 4 months ago

Hi @arbimo , I've looked a bit into it and it seems the correct solution.

Indeed, lower in the code there is this check:

  else:  # expression longer than 3, must be a parameters list
    for e in exp:
        assert isinstance(e, List) or isinstance(
            e, ParseResults
        ), f"expression {exp} is expected to be a parameters list, but it's not"
        pass

so parameters can be either Lists or ParseResults.

Thanks for finding and fixing this!

arbimo commented 3 months ago

@Framba-Luca Thanks for the feedback, I'll ask for it to be merged then ;)