Skip to content

Conversation

ldionne
Copy link
Member

@ldionne ldionne commented Oct 9, 2025

That command had a few issues, which I uncovered when I tried updating the Click dependency. First, it didn't specify the --delete-order parameter as being a multi-parameter, leading to the default value [] being interpreted as a string with value "[]", i.e. a non-empty string.

This led to the tool always trying to delete an order with value "[]", which is nonsensical. For some reason, that only happens with newer Click version.

This patch adds test coverage for --delete-order and makes it a multi valued parameter, which was obviously always the intent. It also improves the documentation for the various parameters, which was unclear in an attempt to be terse.

That command had a few issues, which I uncovered when I tried updating
the Click dependency. First, it didn't specify the --delete-order
parameter as being a multi-parameter, leading to the default value []
being interpreted as a string with value "[]", i.e. a non-empty string.

This led to the tool always trying to delete an order with value "[]",
which is nonsensical. For some reason, that only happens with newer
Click version.

This patch adds test coverage for --delete-order and makes it a multi
valued parameter, which was obviously always the intent. It also improves
the documentation for the various parameters, which was unclear in an
attempt to be terse.
@ldionne ldionne requested a review from lukel97 October 9, 2025 20:34
@click.option("--database", default="default", show_default=True,
help="database to modify")
@click.option("--testsuite", required=True, help="testsuite to modify")
@click.option("--tmp-dir", default="lnt_tmp", show_default=True,
Copy link
Member Author

Choose a reason for hiding this comment

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

This was unused: probably copy-pasted from lnt create

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.

1 participant