-
Notifications
You must be signed in to change notification settings - Fork 87
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
Feature/Analytics #279
Feature/Analytics #279
Conversation
7aac214
to
3dcdcb4
Compare
@alallema, I had some trouble testing the presence of the One thing I thought was to add the header into the Let me know your thoughts :) |
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.
Except for one question LGTM! 🌮
func GetQualifiedVersion() (qualifiedVersion string) { | ||
return getQualifiedVersion(VERSION) | ||
} | ||
|
||
func getQualifiedVersion(version string) (qualifiedVersion string) { | ||
return fmt.Sprintf("Meilisearch Go (v%s)", version) | ||
} |
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.
What is the point of writing to function instead of one? Is it for having a public and a private one?
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.
Yeah, I could add just one function but I was thinking about how to test it, so I created two functions one with the possibility to add a version (private) and another used in the app (public).
This way I could test the format and I could guarantee that the user will always send the right value "VERSION" to the function.
Because I could let this responsibility to the caller:
request.Header.Set("User-Agent", GetQualifiedVersion(VERSION))
But how can I ensure by tests the user is using the VERSION
and not something else?
PS: I still need to understand more about the Go concepts to improve this 😅, feel free to point a better solution :)
I think the best thing to do is to leave it as it is for now until a better solution is found, if it exists or to open a way to ask for the community's help in this matter |
3dcdcb4
to
c54697b
Compare
bors merge |
279: Feature/Analytics r=alallema a=brunoocasali - Add `User-Agent` inside the pre-defined headers Add Go support as requested here meilisearch/integration-guides#150 I tried other options, but they do not fit so well: - https://pkg.go.dev/debug/buildinfo (does not work very well :/ has a lot of issues golang/go#29814, golang/go#33975 - https://github.com/ahmetb/govvv (I think it is not an option since it seems to be not maintained anymore). Co-authored-by: Bruno Casali <brunoocasali@gmail.com> Co-authored-by: Amélie <alallema@users.noreply.github.com>
This PR was included in a batch that successfully built, but then failed to merge into main. It will not be retried. Additional information: {"message":"At least 1 approving review is required by reviewers with write access.","documentation_url":"https://docs.github.com/articles/about-protected-branches"} |
bors merge |
280: Add a new Github workflow step to release r=alallema a=brunoocasali Since the adoption of this new release system *(now we need to update the version of the constant defined in the version.go file). We now need to validate that we are following this step. Related to #279 Co-authored-by: Bruno Casali <brunoocasali@gmail.com> Co-authored-by: Amélie <alallema@users.noreply.github.com>
286: Changes related to the next Meilisearch release (v0.27.0) r=curquiza a=meili-bot Related to this issue: meilisearch/integration-guides#190 This PR: - gathers the changes related to the next Meilisearch release (v0.27.0) so that this package is ready when the official release is out. - should pass the tests against the [latest pre-release of Meilisearch](https://github.com/meilisearch/meilisearch/releases). - might eventually contain test failures until the Meilisearch v0.27.0 is out.⚠️ This PR should NOT be merged until the next release of Meilisearch (v0.27.0) is out. _This PR is auto-generated for the [pre-release week](https://github.com/meilisearch/integration-guides/blob/master/guides/pre-release-week.md) purpose._ Done: - #287 - #289 - #290 - #291 - #294 - #293 # Release **:warning:** The go package didn't need a version update CI will publish the package once the `Publish release` will be done. However, a version file exists and this is only for analytics but it already is on the next version (the v0.19.1). This version makes this package compatible with MeiliSearch v0.27.0🎉 Check out the changelog of [MeiliSearch v0.27.0](https://github.com/meilisearch/MeiliSearch/releases/tag/v0.27.0) for more information about the changes. ## 🚀 Enhancements * Feature/Analytics (#279) `@brunoocasali` * Add new methods for the new typo tolerance settings #294 `@alallema` `Index.GetTypoTolerance()` `Index.UpdateTypoTolerance(params)` `Index.ResetTypoTolerance()` * Ensure nested field support #290 `@alallema` * Add new search parameters highlightPreTag, highlightPostTag and cropMarker #291 `@alallema` Co-authored-by: meili-bot <74670311+meili-bot@users.noreply.github.com> Co-authored-by: Amélie <alallema@users.noreply.github.com> Co-authored-by: alallema <amelie@meilisearch.com>
User-Agent
inside the pre-defined headersAdd Go support as requested here meilisearch/integration-guides#150
I tried other options, but they do not fit so well: