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

Isolate Jersey dependency #51

Merged
merged 2 commits into from
Oct 2, 2014

Conversation

basoko
Copy link
Contributor

@basoko basoko commented Aug 1, 2014

I have done a small refactor with the aim of isolate the Jersey dependency.

@nikoloff
Copy link
Contributor

nikoloff commented Aug 1, 2014

I like this proposal.
I will touch the methods visibility and will build a new version with it.

thx

@basoko
Copy link
Contributor Author

basoko commented Aug 1, 2014

Ok @nikoloff but if you want I can change the methods visibility that you think need a change and update the pull request.

Regards.

@nikoloff
Copy link
Contributor

nikoloff commented Aug 1, 2014

Yes thx @basoko , that will be great.
In the services package the utilities classes ParameterMap and HttpClient with it's methods should be final with package visibility.

@basoko
Copy link
Contributor Author

basoko commented Aug 2, 2014

Sorry @nikoloff, I'm not sure about what you mean because ParameterMap is final and HttpClient is an interface so it can not be final.
I can't change the package visibility because the HttpClient interface is needed in the PaymillContext class, so it has to have public visibility, and at the same time it depends on ParameterMap, so this last one should also be public.

@stoilkov
Copy link
Contributor

stoilkov commented Aug 7, 2014

@basoko sorry about the merge delay, we will take care of both pull requests next week. Again, thanks for the contribution.

@basoko
Copy link
Contributor Author

basoko commented Sep 2, 2014

Hi @nikoloff, @stoilkov is this PR alive?

@stoilkov
Copy link
Contributor

stoilkov commented Sep 3, 2014

@basoko sorry about the delay. will take care of this today.

@basoko
Copy link
Contributor Author

basoko commented Sep 3, 2014

@stoilkov Don't worry, there's no rush. I only wanted to know if there's something I can do to improve it.

@stoilkov stoilkov merged commit 79ab6e9 into paymill:master Oct 2, 2014
stoilkov added a commit that referenced this pull request Oct 2, 2014
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