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

Itemize paper references again. #209

Merged
merged 8 commits into from
Jan 11, 2024

Conversation

jendrikseipp
Copy link
Contributor

This requires a new txt2tags version, which we install via pip rather than bundling it.

@jendrikseipp
Copy link
Contributor Author

I noticed that the autodoc tox step is not called for PRs. To enable it, I had to remove the build.py call from autodoc.py. But this actually brings the test more in line with the other tox tests, since they all assume that the planner is already built.

@FlorianPommerening
Copy link
Member

I think not calling autodoc on each PR was by design: we certainly do not want to update the wiki before a PR is merged, and as a dry run, I don't really see the point because there is no way to see what the changes would look like in the wiki. It's also no "test" that can fail in a meaningful way, is it? I think running it on PRs is unnecessary work that just makes the actions take longer, but I don't feel too strongly about it.

The change to install txt2tags with pip rather than us providing a version in external, and the one about not building the planner anymore mean that is is more complicated now to call autodoc on the command line. Maybe not the most common use case, but I have done that in the past. For that, I think it would be useful to at least test the assumptions and print a useful error message. Something like "txt2tags not found, use 'pip install requirements.txt' to install it" (is that the right command?) for the dependency. For the build you could also add a message or just keep the build method and call it if the planner binary is not present. The new dependency should also be documented, probably here https://www.fast-downward.org/ForDevelopers/DevelopmentSetup

@jendrikseipp
Copy link
Contributor Author

The autodoc tox test runs in a fraction of a second. And it failed in a meaningful way for me while working on this issue, so I think it's good to run the test.

If the binary is missing during an autodoc.py call, we automatically get "Could not find build 'release' at /home/jendrik/projects/Downward/downward/builds/release/bin. Please run './build.py release'." from the driver already. For the case where txt2tags is missing, I added an error message.

I wouldn't add the txt2tags dependency to the wiki dev instructions since tox installs it transparently.

Good to merge?

@FlorianPommerening
Copy link
Member

Fine with me. I'd still add the dependency to the wiki because the tox installation doesn't help when using the script directly.

@jendrikseipp jendrikseipp merged commit e940632 into aibasel:main Jan 11, 2024
12 checks passed
@jendrikseipp jendrikseipp deleted the itemize-papers branch January 11, 2024 21:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants