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

Proposed Cleanup items for BeanPropertyIntrospector #133

Open
wants to merge 7 commits into
base: 2.17
Choose a base branch
from

Conversation

Shounaks
Copy link
Contributor

@Shounaks Shounaks commented Mar 10, 2024

A small effort to reduce complexity.

P.S. sorry for formatting, 😂

Reasoning:
BeanPropertyIntrospector is the brain of this library, and it does too much with too little documentation :

  1. It does not save the state,
  2. It has only 2 methods getting used, while other are helper methods, so only those 2 should be exposed as static
  3. this class is self sufficient. i.e. it does not rely on other class or has to generate new object every time it will get used.

Pending Items

  • test cases passed
  • adding javadocs for methods

@cowtowncoder
Copy link
Member

This will need to wait past 2.17 release, considered for 2.18.

But one quick note: if changing functionality to static methods, sub-classing no longer possible.
I think that I may have intended this to be a possible extension point, so developers could replace it with a sub-class implementation with suitable overrides.
If so, I wouldn't want to change it from regular class into containers of static methods.

@cowtowncoder
Copy link
Member

Ah. Actually... ok, no. I was wrong: there is no way to override it, regardless of my original thinking. Annotation-support package actually has AnnotationBasedIntrospector which ideally should be sub-class, I think.

So there is room for re-considering what to do here. After 2.17.0 release :)

@Shounaks Shounaks force-pushed the 2.17-bean-property-introspector-cleanup branch from 8cc62cf to 2d2c186 Compare March 16, 2024 12:12
@Shounaks Shounaks marked this pull request as ready for review March 16, 2024 12:26
@Shounaks
Copy link
Contributor Author

Can be optimized a bit more, and JavaDocs are remaining.

public BeanPropertyIntrospector() { }

public static BeanPropertyIntrospector instance() { return INSTANCE; }
public final class BeanPropertyIntrospector {
Copy link
Member

Choose a reason for hiding this comment

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

probably can't make a class final in a patch - needs a minor release ideally


public POJODefinition pojoDefinitionForDeserialization(JSONReader r, Class<?> pojoType) {
return _introspectDefinition(pojoType, false, r.features());
public static POJODefinition pojoDefinitionForDeserialization(JSONReader r, Class<?> pojoType) {
Copy link
Member

Choose a reason for hiding this comment

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

can't change a public method to be static in a patch

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I get your concern, this change can have unforeseen side-effects.
Currently this is just a proof of concept, to simplify and cleanup this class.

we can take items from here individually (if not all). or not use this at all (whatever is feasible)

@Shounaks
Copy link
Contributor Author

I gave it a bit of thought, I believe the documentation is good enough, so not changing anymore.

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