Skip to content

feat(java): add algolia user agent APIC-338 #347

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

Merged
merged 6 commits into from
Apr 8, 2022
Merged

feat(java): add algolia user agent APIC-338 #347

merged 6 commits into from
Apr 8, 2022

Conversation

millotp
Copy link
Collaborator

@millotp millotp commented Apr 7, 2022

🧭 What and Why

🎟 JIRA Ticket: APIC-338

Add User-Agent specification in the doc to always have the same format,
and implement it in the java client, as an optional parameter.

🧪 Test

Test the ua in the playground by adding a System.out.println(ua.toString()) in the ApiClient.java constructor.

@millotp millotp requested a review from a team April 7, 2022 09:18
@millotp millotp self-assigned this Apr 7, 2022
@millotp millotp requested review from eunjae-lee and shortcuts and removed request for a team April 7, 2022 09:18
@netlify
Copy link

netlify bot commented Apr 7, 2022

Deploy Preview for api-clients-automation canceled.

Name Link
🔨 Latest commit a703a16
🔍 Latest deploy log https://app.netlify.com/sites/api-clients-automation/deploys/62500a611a5cbd00080381b8

@algolia-bot
Copy link
Collaborator

algolia-bot commented Apr 7, 2022

✗ The generated branch has been deleted.

If the PR has been merged, you can check the generated code on the generated/main branch.

Copy link
Member

@shortcuts shortcuts left a comment

Choose a reason for hiding this comment

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

This looks good! (and thanks for the docs)

I believe we have tests for this in the client tests btw


The version is optional for segments.

The resulting User Agent is the concatenation of `base`, `client`, and all the `segments`.
Copy link
Member

Choose a reason for hiding this comment

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

Maybe an example of output could help

Copy link
Member

Choose a reason for hiding this comment

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

and the link to this PR


public String addSegment(Segment seg) {
String segment = seg.toString();
if (segments.contains(segment)) return finalValue;
Copy link
Member

Choose a reason for hiding this comment

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

non blocking: I did not saw other one liners in this codebase, we might want to stick to curly brackets for consistency

@millotp millotp requested a review from shortcuts April 7, 2022 16:05
Copy link
Member

@shortcuts shortcuts left a comment

Choose a reason for hiding this comment

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

Looks good! Gg

@millotp millotp merged commit 97c1aaf into main Apr 8, 2022
@millotp millotp deleted the feat/ua-java branch April 8, 2022 12:01
shortcuts pushed a commit that referenced this pull request Apr 22, 2022
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