pucrs-automated-planning / pddl-parser

:snake: Classical Planning in Python
GNU General Public License v3.0
83 stars 23 forks source link

Split up into library. Added setup.py. #15

Closed fjulian closed 2 years ago

fjulian commented 2 years ago

To allow using the parser from other projects, this PR proposes to move the code into a python package. A setup.py is added to allow installing the package. Scripts components are removed from the modules and added to separate scripts.

Maumagnaguagno commented 2 years ago

Hello, could you remove the .gitignore file. I would like to avoid having PRs related to adding a new file extension from time to time, see #12.

Is it common practice to move the main code blocks to separate scripts? I personally prefer them together with their related code, to show usage and so on.

EDIT: The #!/usr/bin/env python3 shebangs make it look like a Python 3 only project, but it still works with Python 2. Not sure they are needed, I would avoid.

fjulian commented 2 years ago

Thanks for the fast feedback. I removed the gitignore and changed the shebangs to python to be more general.

I would say it is common practice to not include code intended to be run directly in a module. See for example here: https://realpython.com/lessons/scripts-modules-packages-and-libraries/ However, if you have a strong preference for having the script code with the respective class definition, I can happily move them back.

Btw thanks for making this library available! It's very useful.

Maumagnaguagno commented 2 years ago

Thanks :smile:, I originally did this for my Ruby planner, so it does not follow a Python style sometimes. A rewrite was required to make it consistent with other Python code being used/taught in the university.

I am far more used with the main block to be within the module because of Ruby, where this distinction does not exist. I would like to keep it the way it currently is for a few reasons: history/git blame and less files/beginner friendly. Would that break the setup or just the path would be changed?

I also would like to know if the name and package folder must match in setup.py, otherwise we could use the same name as the one used in the README "PDDL Parser" or the git one "pddl-parser". Not sure which one makes more sense :confused:.

fjulian commented 2 years ago

Okay, moved the script code back now.

Regarding naming, I would keep it at pddl_parser. Package names should be lower case, and hyphens and spaces are not allowed, only underscores. See here: https://peps.python.org/pep-0008/#package-and-module-names

Maumagnaguagno commented 2 years ago

Okay, the naming remains as "pddl_parser". Oh, I forgot to tell you about the paths in the README and GitHub Actions. I think this is a small thing that I can easily fix, no need to worry.

Sorry for the trouble, it is hard to keep things simple and useful at the same time.

I see you did something to ignore errors related to other requirements, I think there is a simpler solution to that. I will investigate that later.

Maumagnaguagno commented 2 years ago

Hey @fjulian, do you know of a way to execute the parser and planner without installing? That would save me some time during development, while being able to keep the README examples simpler.

fjulian commented 2 years ago

I would recommend to install with pip install -e .. This causes the package to be installed in editable mode, so even if you make changes, you don't need to reinstall, which keeps the installation very much out of the way. You only need to do it once in the beginning.

Maumagnaguagno commented 2 years ago

Nice, thanks @fjulian

Maumagnaguagno commented 2 years ago

I made a few more modifications to make it possible to use the parser and planner without installation. Sometimes students work in environments that limit what can be installed. I made a test project and everything seems to work once installated, so it should cover both use cases. Can you confirm this still works for you?

fjulian commented 2 years ago

The setup.py has syntax errors: there are 2 commas missing in lines 7 and 11. Also, it's good practice to have an empty line at the end of any file. Otherwise LGTM.

Maumagnaguagno commented 2 years ago

Oh, my mistake. I tested before making this question to you, and then decided to fill the setup. Thanks for the feedback.