Skip to content
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

LUCENE-9902 Minor fixes to the faceting API #62

Merged
merged 3 commits into from
Apr 6, 2021

Conversation

gautamworah96
Copy link
Contributor

Changes:

  1. Update getOrdinal(String, String[]) call to getOrdinal(String,
    String...)
  2. Update getValue(int) API to be protected

Description

I was using the public int getOrdinal(String dim, String[] path) API for a single path String and found myself creating an array with a single element. We can start using variable length args for this method.

I also propose this change:
I wanted to know the specific count of an ordinal using using the getValue API from IntTaxonomyFacets but the method is private. It would be good if we could change it to protected so that users can know the value of an ordinal without looking up the FacetLabel and then checking its value.

Solution

Changed the function definition and access modifier.
Checked that no other method used the String[] parameter. Also checked that other methods that deal with ordinals and values in IntTaxonomyFacets are also protected

Tests

Already existing tests cover these methods.

Checklist

Please review the following and check all that apply:

  • I have reviewed the guidelines for How to Contribute and my code conforms to the standards described there to the best of my ability.
  • I have created a Jira issue and added the issue ID to my pull request title.
  • I have given Lucene maintainers access to contribute to my PR branch. (optional but recommended)
  • I have developed this patch against the main branch.
  • I have run ./gradlew check.
  • I have added tests for my changes.

Gautam Worah added 2 commits April 3, 2021 14:34
Changes:
1. Update getOrdinal(String, String[]) call to getOrdinal(String,
   String...)
2. Update getValue(int) API to be protected
Copy link
Member

@mikemccand mikemccand left a comment

Choose a reason for hiding this comment

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

Looks great and makes sense! Thank you for your attention to these details @gautamworah96.

Could you maybe add a CHANGES.txt entry, under the Lucene 8.9.0 section?

Copy link
Member

@mikemccand mikemccand left a comment

Choose a reason for hiding this comment

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

@mikemccand mikemccand merged commit efeea0b into apache:main Apr 6, 2021
janhoy pushed a commit to cominvent/lucene that referenced this pull request May 12, 2021
mikemccand pushed a commit to mikemccand/lucene that referenced this pull request Sep 3, 2021
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