Skip to content

Comments

Add support for text-separator customization#537

Merged
meili-bors[bot] merged 5 commits intomeilisearch:mainfrom
Fronix:feature/482-text-separator
May 22, 2024
Merged

Add support for text-separator customization#537
meili-bors[bot] merged 5 commits intomeilisearch:mainfrom
Fronix:feature/482-text-separator

Conversation

@Fronix
Copy link
Contributor

@Fronix Fronix commented May 21, 2024

Pull Request

Related issue

Fixes #482

What does this PR do?

  • Adds support for the text-separator settings

PR checklist

Please check if your PR fulfills the following requirements:

  • Does this PR fix an existing issue, or have you listed the changes applied in the PR description (and why they are needed)?
  • Have you read the contributing guidelines?
  • Have you made sure that the title is accurate and descriptive of the changes?

Thank you so much for contributing to Meilisearch!

@Fronix Fronix force-pushed the feature/482-text-separator branch from 16b3189 to 133cf52 Compare May 21, 2024 10:55
Copy link
Collaborator

@ahmednfwela ahmednfwela left a comment

Choose a reason for hiding this comment

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

Thanks for the PR

One change I would like to suggest, is that in general it's better to use IEnumerable<...> for function inputs and List<...> for function outputs

so for example:

public async Task<List<string>> GetSeparatorTokensAsync(CancellationToken cancellationToken = default)

and

public async Task<TaskInfo> UpdateNonSeparatorTokensAsync(IEnumerable<string> nonSeparatorTokens, CancellationToken cancellationToken = default)

@Fronix
Copy link
Contributor Author

Fronix commented May 21, 2024

Thanks for the PR

One change I would like to suggest, is that in general it's better to use IEnumerable<...> for function inputs and List<...> for function outputs

so for example:

public async Task<List<string>> GetSeparatorTokensAsync(CancellationToken cancellationToken = default)

and

public async Task<TaskInfo> UpdateNonSeparatorTokensAsync(IEnumerable<string> nonSeparatorTokens, CancellationToken cancellationToken = default)

I agree 👍

@Fronix Fronix force-pushed the feature/482-text-separator branch from 9dd1899 to 6979f66 Compare May 21, 2024 15:27
@ahmednfwela
Copy link
Collaborator

bors merge

@meili-bors
Copy link
Contributor

meili-bors bot commented May 22, 2024

Build succeeded:

@Fronix Fronix closed this May 22, 2024
@meili-bors meili-bors bot merged commit 965b7e1 into meilisearch:main May 22, 2024
@Fronix Fronix deleted the feature/482-text-separator branch May 22, 2024 22:58
@curquiza curquiza added the enhancement New feature or request label Jul 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[v1.4] Support text-separator customization

3 participants