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.

Sign up for GitHub

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

Jump to bottom

Improved DefaultFactory#create(Class<T>) #1499

Merged
merged 3 commits into from
Dec 15, 2021
Merged

Improved DefaultFactory#create(Class<T>) #1499

merged 3 commits into from
Dec 15, 2021

Conversation

rgoldberg
Copy link
Contributor

@rgoldberg rgoldberg commented Dec 1, 2021 •
edited
Loading

Improved DefaultFactory#create(Class<T>):

  • Only handle cls argument being a Map or subtype differently than normal if cls is an interface
  • Create TreeMap if cls argument is SortedMap or sub-interface
  • Simplified normal instantiation:
    • no longer call deprecated method
    • only try setAccessible(true) if it might make a difference

remkop
remkop requested changes Dec 1, 2021
View reviewed changes
Copy link
Owner

@remkop remkop left a comment

Choose a reason for hiding this comment

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

Thank you; can you look at my feedback and let me know your thoughts?

src/main/java/picocli/CommandLine.java Show resolved Hide resolved
src/main/java/picocli/CommandLine.java Show resolved Hide resolved
@rgoldberg
DefaultFactory#create(Class<T>):
0911ac5 
Only handle Map differently than normal if the Map is an interface

Handle SortedMap
@rgoldberg
DefaultFactory#create(Class<T>):
4872dbd 
Simplified normal instantiation
@rgoldberg
DefaultFactory#create(Class<T>):
e43ccf0 
Reinstated use of `Class#newInstance()` instead of `Constructor#newInstance()`.

Only use `Constructor#setAccessible(true)` if `IllegalAccessException` thrown.
@rgoldberg
Copy link
Contributor Author

I pushed the requested changes.

@remkop remkop added this to the 4.6.3 milestone Dec 15, 2021
@remkop remkop merged commit 2c6747e into remkop:master Dec 15, 2021
@remkop
Copy link
Owner

remkop commented Dec 15, 2021

Merged. Thank you for the contribution!

remkop added a commit that referenced this pull request Dec 15, 2021
@remkop
[#1499] update RELEASE-NOTES
2c592af
@rgoldberg rgoldberg deleted the default-factory-create-maps branch December 15, 2021 08:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@remkop remkop remkop requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
4.6.3
Development

Successfully merging this pull request may close these issues.
2 participants
@rgoldberg @remkop