-
Notifications
You must be signed in to change notification settings - Fork 33
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
Introducing: Java client v3 #540
Conversation
algoliasearch/src/main/java/com/algolia/search/clients/AlgoliaConfig.java
Outdated
Show resolved
Hide resolved
algoliasearch/src/main/java/com/algolia/search/clients/InsightsConfig.java
Outdated
Show resolved
Hide resolved
algoliasearch/src/main/java/com/algolia/search/clients/AnalyticsClient.java
Outdated
Show resolved
Hide resolved
algoliasearch/src/main/java/com/algolia/search/clients/InsightsConfig.java
Outdated
Show resolved
Hide resolved
algoliasearch/src/main/java/com/algolia/search/helpers/QueryStringHelper.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Really nice improvements 👍
algoliasearch/src/main/java/com/algolia/search/clients/AnalyticsClient.java
Outdated
Show resolved
Hide resolved
algoliasearch/src/main/java/com/algolia/search/clients/AlgoliaConfig.java
Outdated
Show resolved
Hide resolved
algoliasearch/src/main/java/com/algolia/search/clients/AnalyticsConfig.java
Outdated
Show resolved
Hide resolved
algoliasearch/src/main/java/com/algolia/search/clients/InsightsConfig.java
Outdated
Show resolved
Hide resolved
algoliasearch-common/src/main/java/com/algolia/search/Defaults.java
Outdated
Show resolved
Hide resolved
ab0ae6f
to
f2b2920
Compare
ping @BenoitPerrot ! :) Integrated some of the improvements we discussed yesterday. |
2cc3db1
to
2047de5
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Superficial review considering the changeset's size, but still a few (hopefully useful) comments 🙃
@@ -4,10 +4,10 @@ set -e | |||
|
|||
if [[ "${JAVA_VERSION}" = "8" ]]; then | |||
if [ "$TRAVIS_PULL_REQUEST" != "false" ] && [[ ! "$TRAVIS_PULL_REQUEST_SLUG" =~ ^algolia\/ ]]; then | |||
eval $(./algolia-keys export) && mvn clean test jacoco:report jacoco:report-aggregate -B && mvn -pl algoliasearch-tests coveralls:report -B; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you choose to stop reporting Code coverage?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still jacoco
in pom.xml
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nop I don't want to drop it! I removed it temporarily at the begining of the project. I'll add it back in a few days :)
src/test/java/com/algolia/search/integration/insights/InsightsTest.java
Outdated
Show resolved
Hide resolved
import java.util.HashMap; | ||
import java.util.Map; | ||
|
||
public class HttpRequest { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could benefit Nullability annotations (same for HttpResponse
) :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same for any model which would be manipulated by our users, when its fields are not covered by methods with Nullability annotations
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you please elaborate? 🤔 Do you mean to add annotations on the constructors parameters?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean adding annotations wherever our users might benefit it:
- Constructor parameters of classes they could instantiate (not convinced it would be relevant here, as I don't think they have to create a
new HTTPRequest()
anytime) - Annotations on public fields / public {g,s}etters that the users would manipulate to avoid unnecessary null-checks (e.g. if
headers
is nevernull
, would markgetHeaders()
as@NonNull
)
I think this can be applied more consistently in some classes, e.g. :
new Variant(...)
takes a@NonNull String index
in constructor
->Variant.getIndex()
could be annotated as@NonNull
->Variant.setIndex()
should have its param annotated as@NonNull
You might want to use Intellij's Infer Nullity
to quickly generate consistent annotations based on your current codebase 🙃
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the clarification! Neat idea. I'll look at it.
src/main/java/com/algolia/search/models/apikeys/AddApiKeyResponse.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Provided you resolve the last unsolved discussions, LGTM 👌
Thanks! Still need to have a look at the Nullability annotations for some POJOs :) |
Added extraHeaders support Added extraQueryParams support Added a Better exception handling Improved the recursive retry with thenCompose https://github.com/algolia/algoliasearch-client-java-2/projects/1#card-17598839 https://github.com/algolia/algoliasearch-client-java-2/projects/1#card-16731548 https://github.com/algolia/algoliasearch-client-java-2/projects/1#card-16731615
…nd ensure all steps are run in separate order
- Added QueryBase with CRTP to handle Query inheritance and to prevent down-casting for the children
…entral publishing)
Removing slf4j allow to remove on more dependency of the library and to simplify the shading process. - Added: JUL in HttpTransport with Log.FINEST level - Removed: SLF4J and Logback from pom.xml - Updated: logging.properties To activate JUL please set in logging.properties .level=FINEST com.algolia.search.HttpTransport=FINEST java.util.logging.ConsoleHandler.level=FINEST and set: -Djava.util.logging.config.file="logging.properties-path" More information about JUL: http://java.ociweb.com/mark/programming/JavaLogging.html
- Removed org.apache.http.level=WARNING from algoliasearch-core - Added org.apache.http.level=WARNING in algoliasearch-apache file
The query parameter aroundPrecision can now handle a JSON format; { "aroundPrecision": [ {"from":0, "value":10}, {"from":100, "value":1000}, ... ] }
In order to respect fluent API design used by other setters. Reported in #571.
In order to respect fluent API design used by other setters. Reported in #571.
Introducing Java Client v3 ☕️
ChangeLog/ New features
Maven
To get the beta version add this to your pom.xml
Less dependencies!
Injectable HttpCient
The default HttpClient is httpasyncclient from Apache. If you want another one, just implement the
IHttpRequester
interface with your favorite HttpClient and inject it!New test suite!
Asynchronous and synchronous methods in the same class!
Before, asynchronous and synchronous methods were split in two different classes. Now, they are reunited in the same class!
All asynchronous methods are suffixed with the
async
keyword and return aCompletableFuture
.Useful helper: waiTask()
All Algolia's asynchronous operations can now be awaited like this:
Static factories
We added static factories to Algolia's POJOs such as
Synonyms
,Rules
to help you build valid Algolia's objects.Getting started locally:
Clone the repo and checkout on the
develop
branch.Build the project
mvn clean test