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

Exempt picocli.CommandLine.Option#names annotation attribute from reordering #1056

Merged
merged 1 commit into from
Mar 4, 2024

Conversation

rickie
Copy link
Member

@rickie rickie commented Feb 26, 2024

As part of checkstyle/checkstyle#14137 I'm enabling the LexicographicalAnnotationAttributeListing check. I encountered a problem related to this: https://github.com/PicnicSupermarket/error-prone-support/blob/master/integration-tests/checkstyle-init.patch#L78.

Therefore I'm filing this PR.

@rickie rickie added this to the 0.16.0 milestone Feb 26, 2024
Copy link

Looks good. No mutations were possible for these changes.
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

Copy link
Member

@Stephan202 Stephan202 left a comment

Choose a reason for hiding this comment

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

Tnx @rickie! I do seem to recall that this parameter cares about the order in which names are listed, but I forgot the details, and the documentation seems silent about this. What goes wrong without this change?

(Sorry for asking rather than checking myself; rather short on time atm 😬.)

@rickie
Copy link
Member Author

rickie commented Feb 29, 2024

As discussed offline; for some CLI's, like the one in Checkstyle, it's just that a specific order is required. The options are expected or sorted in a specific order and therefore we shouldn't want to change this.

@rickie rickie requested a review from Stephan202 February 29, 2024 14:24
Copy link
Member

@Stephan202 Stephan202 left a comment

Choose a reason for hiding this comment

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

More explicitly: the order in which flags are listed influences the order in which they are rendered in the generated help text. That is, there's a clear functional impact to the order.

@rickie rickie force-pushed the rossendrijver/commandline_name branch from 5564444 to 1bdf1c4 Compare March 4, 2024 09:03
Copy link

github-actions bot commented Mar 4, 2024

Looks good. No mutations were possible for these changes.
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

Copy link

sonarqubecloud bot commented Mar 4, 2024

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

Copy link
Contributor

@mohamedsamehsalah mohamedsamehsalah left a comment

Choose a reason for hiding this comment

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

Clean 🥇

@rickie
Copy link
Member Author

rickie commented Mar 4, 2024

Suggested commit message:

Exempt `picocli.CommandLine.Option#names` annotation attribute from reordering (#1056)

@rickie
Copy link
Member Author

rickie commented Mar 4, 2024

@Stephan202 we could go for this as alternative:

Exempt `@picocli.CommandLine.Option#names` attribute from reordering (#1056)

As it will be under 80 chars, WDYT?

@Stephan202
Copy link
Member

The @ may not be obvious. Perhaps just:

Exempt `picocli.CommandLine.Option#names` arguments from reordering (#1056)

@rickie rickie merged commit 110ac01 into master Mar 4, 2024
15 checks passed
@rickie rickie deleted the rossendrijver/commandline_name branch March 4, 2024 10:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

3 participants