search

edulify / play-geolocation-module.edulify.com

This is a geolocation plugin for Playframework
https://edulify.github.io/play-geolocation-module.edulify.com/
Apache License 2.0
22 stars 11 forks source link

Improvement request #7

Closed sovaalexandr closed 8 years ago

sovaalexandr commented 9 years ago

I'd suggest to move away from static calls to service methods or to cache. Why it should be done could be read here: http://java.dzone.com/articles/why-static-bad-and-how-avoid What do I want to achieve while/after this improvement:

If you consider it worthwhile I could handle it in backwards-compatible way and send you a pull-request

megazord commented 9 years ago

@sovaalexandr

Agree with you. It was done this way to keep the plugin consistent with Play way of doing stuff. Since Play 2.4 is moving to use DI, we will also release a version of this plugin that favors DI over static methods.

I will really appreciate if you help me with that, but first let me create a new branch (play-2.4.x) to isolate this change from the master until is is ready to be released. The branch will be created as soon as we release a new version with your other pull requests.

sovaalexandr commented 9 years ago

@megazord

BTW How about version 2.0 that would be based on Java 8 lambdas? I found it extremely useful for code readability.

megazord commented 9 years ago

@sovaalexandr

What do you mean by based on Java 8 lambdas? Expose an API that is lambda friendly or use them internally? Expose an API that is lambda friendly is not that hard and I think we are already there (based on the fact that the plugin interfaces can be classified as functional interfaces).

sovaalexandr commented 9 years ago

@megazord

Yes, I actually meant to use it internally within library and tests

sovaalexandr commented 9 years ago

@megazord

I have prepared a proposition branch on versoin-2.0 Would you like to get a pull request with those changes?

megazord commented 9 years ago

@sovaalexandr sorry for the delay to give you an answer. Please, submit a PR with your changes to branch play-2.4.0.

Thanks.

sovaalexandr commented 9 years ago

@megazord

Hello. Play 2.4 released after I made those changes and now after it's stable I have progress since Thursday, 28th of May of rewriting this. When it will be ready I'll submit pull request. Shure I'll keep your comments in mind while changing code

sovaalexandr commented 9 years ago

Pull request is ready. But while creating it

marcospereira commented 8 years ago

@sovaalexandr I think that most of this issue was fixed in the version 2.0.0. What do you think?

sovaalexandr commented 8 years ago

@marcospereira I agree. This issue should be closed.