Implement __str__ for Domain and Problem

[ssardina]

Proposed changes

String representations of Domain and Problem are currently done in pddl.formatter with functions problem_to_string and domain_to_string

Fixes

Seems to me it is cleaner to have __str__ implemented in Domain and Problem classes and this is what this PR does

• moved code in xxx_to_string functions to __str__ implementations in Domain and Problem classes
• make xxx_to_string just call str
• leave all aux methods in formatter, but make them public as they can be used by other modules (they are now used in __str__ implementations)

Types of changes

What types of changes does your code introduce?
Put an x in the boxes that apply

• Bugfix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to not work as expected)
• Non-breaking refactor change

Checklist

Put an x in the boxes that apply.

• I have read the CONTRIBUTING doc
• Lint and unit tests pass locally with my changes
• I have added tests that prove my fix is effective or that my feature works

Further comments

This is a very small refactoring change.

[francescofuggitti]

Hi @ssardina, thanks for your PR! I see the black style check is still failing. Could you please fix it before this can be approved?

All the rest looks good.

[codecov-commenter]

Codecov Report

Attention: Patch coverage is 97.72727% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 88.37%. Comparing base (4ee8d63) to head (553de64).

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #109      +/-   ##
==========================================
+ Coverage   88.35%   88.37%   +0.02%     
==========================================
  Files          25       25              
  Lines        1794     1798       +4     
  Branches      333      333              
==========================================
+ Hits         1585     1589       +4     
  Misses        149      149              
  Partials       60       60              

Flag	Coverage Δ
unittests	88.37% <97.72%> (+0.02%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files	Coverage Δ
pddl/formatter.py	96.96% <100.00%> (+0.19%)	⬆️
pddl/core.py	93.41% <96.77%> (+0.76%)	⬆️

[francescofuggitti]

See comment above.

[ssardina]

No problem, I will check this and fix it. I need to understand what is happening, which is good, but it won't be done right away, but I am aware of it and will handle it. Thanks @francescofuggitti !

[francescofuggitti]

Sure, no worries. From the error logs, I think it's just a matter of invoking black on the project.

Great, thanks @ssardina!

[ssardina]

@francescofuggitti I think I have fixed the formatting issues in both files. Note that because I have moved code in domain_to_string in formatter.py to core.py, some functions in formatter.py are not "private" anymore, because they are called from core.py so I removed the leading _ in their names, OK? For example:

So, formatter.py now provides a set of tools for formatting 😉

The formatting issue, via black, should be fixed. I think the coverage was not impacted and even seems to have gone up a bit (there seems to be some partial coverage but it was there before). Let me know if I am getting this wrong.

[francescofuggitti]

Hi @ssardina! Thank you for the update. I've approved the pipelines but it seems that the black check is still failing on the core.py module. Is there any issue with it?

Regarding the "private" to "non-private" functions, it's totally fine.

[ssardina]

hi @francescofuggitti .. mmm you are right:

would reformat /home/runner/work/pddl/pddl/pddl/core.py

I found out that my black formatter was a bit newer (version 24) compared with the one in the repo (version 23) and incredibly they do different things!

Basically, with my black I get that core.py is fine, but parser/problem.py is not!

❯ black --version
black, 24.3.0 (compiled: yes)

❯ black pddl --check
would reformat /home/ssardina/git/planning/parsers/pddl.git/pddl/parser/problem.py

Oh no! 💥 💔 💥
1 file would be reformatted, 24 files would be left unchanged.

The change requested for problem.py would be:

❯ black pddl/parser/problem.py --check --diff
--- pddl/parser/problem.py	2024-03-16 04:30:27.755559+00:00
+++ pddl/parser/problem.py	2024-03-18 22:18:51.324753+00:00
@@ -180,13 +180,15 @@
             obj2 = self._objects_by_name.get(args[2])
             return EqualTo(obj1, obj2)
         else:
             name = args[1]
             terms = [
-                Constant(str(_term_name))
-                if self._objects_by_name.get(str(_term_name)) is None
-                else self._objects_by_name.get(str(_term_name))
+                (
+                    Constant(str(_term_name))
+                    if self._objects_by_name.get(str(_term_name)) is None
+                    else self._objects_by_name.get(str(_term_name))
+                )
                 for _term_name in args[2:-1]
             ]
             return Predicate(name, *terms)
 
     def f_exp(self, args):
would reformat pddl/parser/problem.py

Oh no! 💥 💔 💥
1 file would be reformatted.

The repo is using version 23 it seems:

black-check: black==23.3.0,click==8.1.7,mypy-extensions==1.0.0,packaging==24.0,pathspec==0.12.1,pip==24.0,platformdirs==4.2.0,setuptools==69.1.0,wheel==0.42.0

So I downgraded to 23.3.0 and I do get the same as the repo checker:

❯ black --version
black, 23.3.0 (compiled: yes)
Python (CPython) 3.10.12
❯ black pddl --check --diff
--- /home/ssardina/git/planning/parsers/pddl.git/pddl/core.py	2024-03-18 21:22:55.027221 +0000
+++ /home/ssardina/git/planning/parsers/pddl.git/pddl/core.py	2024-03-18 22:24:14.268847 +0000
@@ -218,13 +218,13 @@
         self._domain: Optional[Domain]
         self._domain_name: name_type
         self._domain, self._domain_name = self._parse_domain_and_domain_name(
             domain, domain_name
         )
-        self._requirements: Optional[AbstractSet[Requirements]] = (
-            self._parse_requirements(domain, requirements)
-        )
+        self._requirements: Optional[
+            AbstractSet[Requirements]
+        ] = self._parse_requirements(domain, requirements)
         self._objects: AbstractSet[Constant] = ensure_set(objects)
         self._init: AbstractSet[Formula] = ensure_set(init)
         self._goal: Formula = ensure(goal, And())
         self._metric: Optional[Metric] = metric
         validate(
would reformat /home/ssardina/git/planning/parsers/pddl.git/pddl/core.py

Oh no! 💥 💔 💥
1 file would be reformatted, 24 files would be left unchanged.

OK so a bit surprising but... I will do the changes under 23.3 black version and push the changes.

[ssardina]

OK @francescofuggitti , let's see now, I have applied black 23.3 version to address the issue. I cannot see the check running again automatically, I guess you need to run it manually?

Also, I am a bit confused how one should work with this. The vscode black extension already uses version 24 (which, as seen here, would format files differently from version 23):

This means that I would have to run the black formatter manually outside vscode, which is OK but maybe it would be good to clarify this in the README and be explicit on what version of black needs to be used to pass the checks? Or am I missing something?

[haz]

A wild way for things to break! Not sure what the long-term solution is, but for now we could probably just use a more recent version: https://github.com/AI-Planning/pddl/blob/main/Pipfile#L16

@francescofuggitti @marcofavorito ?

[francescofuggitti]

Yeah, this is a kind of general issue when working with multiple code style checkers (sometimes they do conflict with each other...).
In fact, now isort is not passing...

For this PR, we can try to apply some workaround (ignore some checks, etc). However, for the long-term solution, my proposal is to have the following:

• move all checks to ruff, to avoid issues across style checkers and to manage only one tool
• use pre-commit hooks to ensure that all checks are done at commit time
• provide a more comprehensive CONTRIBUTING.md with a step-by-step guide to set up the dev environment

What do you think?

[haz]

As a pre-commit hook, it’ll start complaining and prevent the commit? Prevent the push? As long as the feedback is clear (I assume ruff would be), then that should make things easier, for sure. Having the block be on the PR review is a cumbersome process…

[francescofuggitti]

As a pre-commit hook, it’ll start complaining and prevent the commit? Prevent the push? As long as the feedback is clear (I assume ruff would be), then that should make things easier, for sure. Having the block be on the PR review is a cumbersome process…

Yes, the pre-commit will directly prevent the commit...

[francescofuggitti]

As of now, the best thing to do would be to install dev dependencies with pipenv install --dev and manually run tox before committing/pushing.

@ssardina, I've taken a closer look at the code, and I guess that to fix this PR you can do the following:

• remove from textwrap import indent from the formatter.py and add it to the core.py
• in the formatter.py, add a docstring to methods in lines 29 and 70
• run isort to sort imports on the core.py module
• run tox to check that everything is ok

[ssardina]

Hi @francescofuggitti , thanks for that summary of what to do.. wow this tox is, or looks, amazing, can I use it with my course assessment and papers? 😆

OK I am close but yet there:

I know why black failed, still have version 24. easy...

need to investigate what is that flake8 and evaluation failed parts though..

[francescofuggitti]

Hi @francescofuggitti , thanks for that summary of what to do.. wow this tox is, or looks, amazing, can I use it with my course assessment and papers? 😆

OK I am close but yet there:

I know why black failed, still have version 24. easy...

need to investigate what is that flake8 and evaluation failed parts though..

Yeah, tox is really cool :)

Usually, when you're debugging tox errors, you may want to run only single pieces like tox -e flake8 or tox -e black-check. The failed evaluation should be fixed once black and flake are ok too. All commands that tox runs are in the tox.ini file in the root folder of the project.

Anyway, I think we're close to success here. To fix flake8 I guess the method def print_predicates_with_types(predicates: Collection): (line 83) is missing the docstring. And for black, as you correctly guessed, it's just a version issue.

Thank you again for putting your effort here, we're really pleased to get contributions from the community 🚀

[francescofuggitti]

Something is moving behind the scenes... #111 👀

[ssardina]

Thanks @francescofuggitti for all the explanations, very useful. 🙏 and no worries on my side, I am actually learning quite a bit! 👍

I can now run tox targetted and I understand the issues it reports; there were 3 issues reported, very nice tools these ones.

[ssardina]

Is it correct that you need to start the GH checks yourself manually right? I cannot see them triggering when I add to the PR

[haz]

Ya, repo maintainers need to approve — it’s to prevent DDoS’ing a repo’s computation with a crazy # of PR’s.

[francescofuggitti]

Everything is green now :)) Fantastic!! Thank you, @ssardina!

[ssardina]

green is nice 💚

Thanks @francescofuggitti & @haz for your patience.. 👍