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

Make AbstractPatriciaTrie public #407

Merged
merged 2 commits into from
Jul 27, 2023

Conversation

vad0
Copy link
Contributor

@vad0 vad0 commented Jul 25, 2023

I extend this class here to make an implementation which works with an AsciiSequenceView instead of String. Package visibility forces me to create a package org.apache.commons.collections4.trie in my project. This hack works, but it prevents me from migrating to the java modules system, which prohibits using the same package names in the different modules. So I think the best solution here wil be to make AbstractPatriciaTrie public.

@Claudenw
Copy link
Contributor

I encountered a similar problem with AbstractPatriciaTrie but ended up not using the Trie.

@garydgregory
Copy link
Member

Seems reasonable to make public unless I am missing something. @aherbert ?

@aherbert
Copy link
Contributor

The use case merits it being made public. Note that if we make it public then it needs some more javadoc as it will now appear in the javadoc API. The types <K, V> are not documented, e.g. are there limitations? One of the constructors has no javadoc, the other has no documentation of the parameters. IIUC the minimal effort would be to copy what is in the superclass AbstractBitwiseTrie for the type parameters and the constructors. I did not check the rest of the rendered javadoc but this should be done for completeness.

@vad0
Copy link
Contributor Author

vad0 commented Jul 26, 2023

Copied some javadocs from the parent class

@codecov-commenter
Copy link

Codecov Report

Merging #407 (b85e9da) into master (99473df) will not change coverage.
The diff coverage is n/a.

@@            Coverage Diff            @@
##             master     #407   +/-   ##
=========================================
  Coverage     81.20%   81.20%           
  Complexity     4625     4625           
=========================================
  Files           290      290           
  Lines         13483    13483           
  Branches       1993     1993           
=========================================
  Hits          10949    10949           
  Misses         1933     1933           
  Partials        601      601           
Files Changed Coverage Δ
...ommons/collections4/trie/AbstractPatriciaTrie.java 53.45% <ø> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@garydgregory garydgregory merged commit 66d6b9e into apache:master Jul 27, 2023
6 checks passed
garydgregory added a commit that referenced this pull request Jul 27, 2023
anantdamle pushed a commit to anantdamle/commons-collections that referenced this pull request Aug 14, 2023
* Make AbstractPatriciaTrie public

* Added javadocs
anantdamle pushed a commit to anantdamle/commons-collections that referenced this pull request Aug 14, 2023
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.

5 participants