Pull Request: https://github.com/aibasel/downward/pull/228

David did another review of the logic changes (the place where we parse validator inputs). One potential problem came up in that review but it is unrelated to this issue, so we'll open a new issue for it. We decided to go ahead with the merge.

Yes, translate/{tests,regression-tests} is certainly not part of the change for this issue. I brought it up because I noticed it in the context of this issue (because it came up when searching for translator code that still uses os.path). I've mentioned them on Discord, let's see if anyone wants to keep the code.

Thanks Malte. I handled your comment by undoing the relevant change. I don't necessarily want someone to check everything. If you are fine with merging, I am as well.

Regarding the tests: I also think they are unused and I'm fine with deleting them, but I don't think this belongs in this issue (which is not about the translator or tests). If it's just about deleting the files, we could do this as a [trivial] commit on main. If it's more work or if someone wants to look into reactivating them, we could create a new issue for this.

I left some comments at the pull request. I couldn't check everything, but also won't have time for that in the near future. So if you'd like someone to check everything, we need a different reviewer.

Regarding the tests, I had a closer look at src/translate/tests and src/translate/regression-tests. In my view, they should be turned into actual tests that can be executed or deleted; in the current unmaintained form I don't think they belong in the repository.

(My understanding is that neither is currently used -- correct me if I'm wrong.)

I opened a pull request for this: https://github.com/aibasel/downward/pull/228

I don't know about the tests but it seems they are not used. 

I tried running them locally: `python -m pytest ./tests/test_scripts.py` worked but `python -m pytest ./tests/test_normalization.py` failed with this error:

E       AssertionError: assert 'Atom @object... =(?Z, ?Z@2).' == 'Atom @object... =(?Z, ?Z@2).'
E         Skipping 421 identical leading characters in diff, use -v to show
E         - ?Y, ?Z, ?Y, ?Z), Atom =(?Y, ?Y@0), Atom =(?Y, ?Y@1), Atom =(?Z, ?Z@2).
E         + ?Y, ?Z, ?Y@1, ?Z@2), Atom =(?Y, ?Y@0), Atom =(?Y, ?Y@1), Atom =(?Z, ?Z@2).
E         ?           ++    ++

As far as I can tell, this if caused by the rule

    prog.add_rule(Rule([pddl.Atom("p", ["?Y", "?Z", "?Y", "?Z"])], pddl.Atom("q", ["?Y", "?Y"])))

where we expect "Atom p(?Y, ?Z, ?Y, ?Z)" in the body, but get "Atom p(?Y, ?Z, ?Y@1, ?Z@2)". Here are the full rules with extra spaces for alignment:

EXPECTED: none Atom q(?Y, ?Y@0) :- Atom p(?Y, ?Z, ?Y  , ?Z  ), Atom =(?Y, ?Y@0), Atom =(?Y, ?Y@1), Atom =(?Z, ?Z@2).
GOT:      none Atom q(?Y, ?Y@0) :- Atom p(?Y, ?Z, ?Y@1, ?Z@2), Atom =(?Y, ?Y@0), Atom =(?Y, ?Y@1), Atom =(?Z, ?Z@2).

With the equality conditions in the end, this actually looks OK to me.

I checked if we used os.path in the translator. We do in translate/tests (only). Is translate/tests actively used? I see it hasn't really been touched since 2016. Should it be removed?

(I thought the tests we do use live all live under "misc".)

Great!

In issue730, we starting using pathlib instead of os.path. This makes the code nicer but started to get into too many places unrelated to issue730, so we decided to do this in its own issue.