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

Adding ScimExtensionUtil.java #57

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from
Open

Conversation

mez5001
Copy link

@mez5001 mez5001 commented Apr 30, 2018

Make it easier to other projects to register a ScimExtension without needing to know the internals of how the ScimRegistry works.

@mez5001 mez5001 requested a review from chrisharm April 30, 2018 20:40
@smoyer64
Copy link
Contributor

smoyer64 commented May 2, 2018

I do like the idea of making it easier ... I think we also need some instructions and a better sample project (TIER is super dated).

* Register multiple extensions through a map
* @param extensions Map containing all of the ScimResources and the accompanying Extensions
*/
public static void registerExtensions(Map<Class<? extends ScimResource>, Collection<Class<? extends ScimExtension>>> extensions) {
Copy link
Contributor

@smoyer64 smoyer64 May 10, 2018

Choose a reason for hiding this comment

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

This method signature is a bit awkward ... the user is going to work just as hard (or harder) wrapping up the Map as you are unwrapping it. Might I suggest that we change the signature of the method on line 24 to:

public static void registerExtension(Class<? extends ScimResource> rClass, Class<? extends ScimExtension>... eClass)

And that we eliminate this more complex method. This would require one line of code for each resource class which I think is reasonable.

@smoyer64
Copy link
Contributor

Talking this through a bit with @chrisharm we realized that since we're suggesting a single method and it's got such simple code, perhaps this would better be effected directly in ScimExtension.

Copy link
Contributor

@chrisharm chrisharm left a comment

Choose a reason for hiding this comment

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

Let's move these methods to the ScimExtentionRegistry

@smoyer64
Copy link
Contributor

@chrisharm it should be just one method ... I still think we need to scrap the one that consumes a Map argument.

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