Skip to content

Issue1223 - #298

Merged
roeger merged 20 commits into
aibasel:mainfrom
FlorianPommerening:issue1223
Jul 27, 2026
Merged

Issue1223#298
roeger merged 20 commits into
aibasel:mainfrom
FlorianPommerening:issue1223

Conversation

@FlorianPommerening

Copy link
Copy Markdown
Member

Adds documentation for the translator options

@roeger roeger left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had a look at the pull request and only have two small comments.

But wouldn't it make sense to split this into two separate pull requests, one for the renaming in the translator and one for the translator option documentation?

Comment thread src/translate/pddl_parser/parsing_functions.py
Comment thread misc/autodoc/generate-translate-docs.py Outdated
except Exception:
pass

return f"### {name}\n{help_text}\n"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This leads to a very ugly table of contents on the right-hand side, where the options are listed with their parameters (and truncated).

Can we augment the subheadings with a short version for the toc? For example:

### `--invariant-generation-max-time INVARIANT_GENERATION_MAX_TIME` {: data-toc-label="--invariant-generation-max-time"}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the idea of having shorter names in the table of contents but I do not know how we could implement this (probably because I do not know how the website generates the table of contents).

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's why I gave the example. I just added the corresponding commit. @FlorianPommerening or @grucla If you think this change is ok, you can from my side merge the pull request.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm fine with merging as well.

@FlorianPommerening

Copy link
Copy Markdown
Member Author

But wouldn't it make sense to split this into two separate pull requests, one for the renaming in the translator and one for the translator option documentation?

We said on Discord that we would treat the renaming as drive-by clean-up unless it needs more discussion. Do you think it does?

@roeger

roeger commented Jul 26, 2026

Copy link
Copy Markdown
Contributor

But wouldn't it make sense to split this into two separate pull requests, one for the renaming in the translator and one for the translator option documentation?

We said on Discord that we would treat the renaming as drive-by clean-up unless it needs more discussion. Do you think it does?

I don't think it is controversial but it feels like half of the change set.

@FlorianPommerening

Copy link
Copy Markdown
Member Author

But wouldn't it make sense to split this into two separate pull requests, one for the renaming in the translator and one for the translator option documentation?

We said on Discord that we would treat the renaming as drive-by clean-up unless it needs more discussion. Do you think it does?

I don't think it is controversial but it feels like half of the change set.

Fine with me, but in this case I suggest we go ahead with this issue before doing the renaming. We'll have to cherry-pick the commits that do the renaming from this branch to a new branch and revert them here. I would treat the renaming as not in scope of the sprint story if we don't do it as "drive-by".

@roeger

roeger commented Jul 27, 2026

Copy link
Copy Markdown
Contributor

Fine with me.

@roeger

roeger commented Jul 27, 2026

Copy link
Copy Markdown
Contributor

I reverted the changes to the translator here and moved them into a separate PR #300.

@roeger
roeger merged commit 7120aa0 into aibasel:main Jul 27, 2026
9 checks passed
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.

3 participants