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

WW-5381 Introduce extension point for MethodAccessor #825

Merged
merged 5 commits into from
Jan 6, 2024

Conversation

kusalk
Copy link
Member

@kusalk kusalk commented Dec 29, 2023

WW-5381

Fairly straight forward again but see my comment below

@@ -87,12 +92,6 @@ protected void setContainer(Container container) throws ClassNotFoundException {
OgnlRuntime.setPropertyAccessor(cls, container.getInstance(PropertyAccessor.class, name));
}

names = container.getInstanceNames(MethodAccessor.class);
Copy link
Member Author

Choose a reason for hiding this comment

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

Now there is one drawback here - applications lose the ability to specify custom method accessors for different types. They will now instead need to use the extension point and do the delegation within this "global" method accessor. I'm not sure there's a way to retain this capability and make it compatible with the current extension point system.

Copy link
Member

Choose a reason for hiding this comment

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

Hmm... so maybe just mark this functionality as @deprecated or remove - once someone starts complain we can re-think how to put back such functionality.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm actually just had another think and we should be able to retain backwards compatibility, the code will be a little ugly though - let me give it a shot

Copy link
Member Author

Choose a reason for hiding this comment

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

I think we are good now :) Hopefully the code makes sense, added some JavaDoc

Copy link
Member

Choose a reason for hiding this comment

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

Looks good to me, thanks!

@kusalk
Copy link
Member Author

kusalk commented Dec 29, 2023

(SonarCloud report is comparing against wrong base branch)

@kusalk kusalk force-pushed the WW-5381-compoundroot-interface-2 branch from 4ae768e to f3c160d Compare January 3, 2024 10:44
Base automatically changed from WW-5381-compoundroot-interface-2 to master January 3, 2024 12:11
@kusalk kusalk marked this pull request as ready for review January 3, 2024 12:12
Copy link

sonarcloud bot commented Jan 5, 2024

Quality Gate Failed Quality Gate failed

Failed conditions

10 Security Hotspots
28.6% Coverage on New Code (required ≥ 80%)
4.1% Duplication on New Code (required ≤ 3%)
E Security Rating on New Code (required ≥ A)
E Reliability Rating on New Code (required ≥ A)

See analysis details on SonarCloud

idea Catch issues before they fail your Quality Gate with our IDE extension SonarLint SonarLint

@kusalk kusalk merged commit 3b76678 into master Jan 6, 2024
8 of 10 checks passed
@kusalk kusalk deleted the WW-5381-method-accessor branch January 6, 2024 15:14
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