Skip to content
This repository has been archived by the owner on Jan 9, 2018. It is now read-only.

Remove WampBuilder.setActionMapping() #29

Open
trumpetinc opened this issue Feb 18, 2014 · 2 comments
Open

Remove WampBuilder.setActionMapping() #29

trumpetinc opened this issue Feb 18, 2014 · 2 comments
Milestone

Comments

@trumpetinc
Copy link
Collaborator

Remove WampBuilder.setActionMapping() and DefaultRPCManager.setActionMapping() methods.

Instead, have an addAll() method that copies the values from an existing mapping.

The current implementation allows a user to accidentally destroy previous configuration.

@ghetolay
Copy link
Owner

We can add an addAll() method but this won't replace the setter need, this is 2 different use case.
setActionMapping() allow user to provider it's own ActionMapping implementation.

For DefaultRPCManager, Initially ActionMapping was a constructor argument but I guess this was causing troubles. Also if user want to manipulate directly DefaultRPCManager he should understand how it works and it's at his own risk (we'll provide complete javadoc).

About the Builder it's suppose to make things easy for the user so I've already thought about :

public RPCManager newRPCManager();
public RPCManager newRPCManager(ActionMapping mapping);

and remove setActionMapping(). That's why there is a javadoc comment.

The jsr356 version is a first draft. Especially WampBuilder, this was done pretty fast and there was no real design thinking behind it. For example embedding multiple builder may be a bad choice.

What we really need is a design reflexion about the whole class and usage more than just a specific method.

@ghetolay ghetolay added this to the JSR356 milestone Feb 19, 2014
@ghetolay ghetolay removed the jsr356 label Feb 19, 2014
@ghetolay ghetolay modified the milestone: JSR356 Feb 19, 2014
@trumpetinc
Copy link
Collaborator Author

ok - do you want to open a new issue to discuss the WampBuilder design/etc... or discuss it here?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

2 participants