Skip to content
This repository has been archived by the owner on Apr 13, 2023. It is now read-only.

fix: allow revinclude to return more than 10 resources #164

Merged
merged 4 commits into from
Feb 1, 2022

Conversation

ssvegaraju
Copy link
Contributor

Issue #154 , if available:

Description of changes:
Added a size parameter to the multisearch used by the inclusion parameters (include and revinclude). Updated test snapshots to include the size parameter.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@ssvegaraju ssvegaraju requested a review from a team as a code owner January 31, 2022 18:57
Copy link
Contributor

@carvantes carvantes left a comment

Choose a reason for hiding this comment

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

Solution is correct. I left a couple of comments to tidy up the implementation.

Also, consider renaming the PR. Something like "fix: allow revinclude to return more than 10 resources".
It's often better to think about the end result instead of the specific internals being changed when thinking of a PR name.

src/elasticSearchService.ts Outdated Show resolved Hide resolved
@github-actions github-actions bot added size/s and removed size/xs labels Feb 1, 2022
@ssvegaraju ssvegaraju changed the title fix: multisearch should return >10 queries fix: allow revinclude to return more than 10 resources Feb 1, 2022
@codecov-commenter
Copy link

codecov-commenter commented Feb 1, 2022

Codecov Report

Merging #164 (28e65e3) into mainline (8c68539) will increase coverage by 0.01%.
The diff coverage is 100.00%.

Impacted file tree graph

@@             Coverage Diff              @@
##           mainline     #164      +/-   ##
============================================
+ Coverage     90.93%   90.94%   +0.01%     
============================================
  Files            35       35              
  Lines          1147     1149       +2     
  Branches        258      258              
============================================
+ Hits           1043     1045       +2     
  Misses          103      103              
  Partials          1        1              
Impacted Files Coverage Δ
src/elasticSearchService.ts 88.23% <ø> (ø)
src/constants.ts 100.00% <100.00%> (ø)
src/searchInclusions.ts 98.16% <100.00%> (+0.01%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8c68539...28e65e3. Read the comment docs.

@ssvegaraju ssvegaraju requested a review from carvantes February 1, 2022 01:20
src/constants.ts Outdated
@@ -26,3 +26,5 @@ export const NON_SEARCHABLE_PARAMETERS = [
export const MAX_ES_WINDOW_SIZE: number = 10000;

export const MAX_CHAINED_PARAMS_RESULT: number = 100;

export const MAX_NUM_ES_HITS: number = 1000;
Copy link
Contributor

Choose a reason for hiding this comment

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

this const is specific to inclusion params. Maybe rename to MAX_INCLUSION_PARAM_RESULTS

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for the name suggestion, I've updated it as such!

@ssvegaraju ssvegaraju requested a review from carvantes February 1, 2022 02:04
@ssvegaraju ssvegaraju merged commit b1e3a1a into mainline Feb 1, 2022
@ssvegaraju ssvegaraju deleted the fix-inclusionSize branch February 1, 2022 02:10
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants