Skip to content

Javadoc: Fix swapped '@' and '+' descriptions in PatternOptionBuilder - #446

Merged
garydgregory merged 3 commits into
apache:masterfrom
oscerd:javadoc-pattern-object-class
Oct 29, 2026
Merged

garydgregory merged 3 commits into
apache:masterfrom
oscerd:javadoc-pattern-object-class

Conversation

@oscerd

@oscerd oscerd commented Oct 28, 2026 •

Copy link
Copy Markdown
Contributor

The table at the top of the PatternOptionBuilder Javadoc has the descriptions of @ and + swapped. @ maps to Object and creates an instance of the named class through its no-argument constructor, while + maps to Class and only loads the class. PatternOptionBuilderTest.testSimplePattern already checks it that way (b@ gives a Vector, f+ gives Calendar.class), so only the docs were wrong.

Since someone reading the old table could pick @ thinking they just get a class name back, I also added a short note below the example and on Converter.OBJECT, Converter.CLASS, TypeHandler.createObject() and TypeHandler.createClass(): they load whatever class the value names (and @/OBJECT also runs its constructor), so they shouldn't be used on values from an untrusted source.

Javadoc only, no behavior change.

  • Read the contribution guidelines for this project.
  • Read the ASF Generative Tooling Guidance if you use Artificial Intelligence (AI).
  • I used AI to create any part of, or all of, this pull request. Which AI tool was used to create this pull request, and to what extent did it contribute? Claude Code (Claude Opus 5.5) was used to analyze the issue and to prepare the change and this description.
  • Run a successful build using the default Maven goal with mvn; that's mvn on the command line by itself.
  • Write unit tests that match behavioral changes, where the tests fail if the changes to the runtime are not applied. This may not always be possible, but it is a best practice. Not applicable: Javadoc only.
  • Write a pull request description that is detailed enough to understand what the pull request does, how, and why.
  • Each commit in the pull request should have a meaningful subject line and body. Note that a maintainer may squash commits during the merge process.

🤖 Generated with Claude Code

The table in the PatternOptionBuilder Javadoc had '@' and '+' the wrong
way round. '@' is the code that creates an object through the no-arg
constructor of the named class, while '+' only loads the class; the
existing tests already check it that way.

While at it, note in Converter.OBJECT, Converter.CLASS,
TypeHandler.createObject() and TypeHandler.createClass() that they load
whatever class the value names, so they shouldn't be used on values
from an untrusted source.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@garydgregory

Copy link
Copy Markdown
Member

@oscerd

Thanks for the update.

Could you adjust the Javadoc saying “options itself is not modified”? Parsing still changes option-group selections, so that’s a bit too broad. Or am I missing something?

Maybe: “Property values are added to the returned CommandLine without changing the values of the registered options.”

@oscerd

oscerd commented Oct 28, 2026

Copy link
Copy Markdown
Contributor Author

You're right, sorry about that. Parsing still updates the option group selections, so that was too broad. The sentence is in #450, I've changed it there to your wording. Thanks!

@garydgregory garydgregory left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Hi @oscerd
What do you think about my suggestions?

oscerd and others added 2 commits October 29, 2026 06:28
Co-authored-by: Gary Gregory <garydgregory@users.noreply.github.com>
Co-authored-by: Gary Gregory <garydgregory@users.noreply.github.com>

@oscerd oscerd left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thanks, Suggestions merged

@garydgregory
garydgregory merged commit cf22582 into apache:master Oct 29, 2026
10 checks passed
@garydgregory

Copy link
Copy Markdown
Member

TY @oscerd , merged 🚀

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