search

mikemccand / stargazers-migration-test

Testing Lucene's Jira -> GitHub issues migration
0 stars 0 forks source link

Move Kuromoji DictionaryBuilder tool from src/tools to src/ [LUCENE-8871] #868

Closed mikemccand closed 5 years ago

mikemccand commented 5 years ago

Currently tests in tools directories are not run as part of the normal testing done by ant test - you have to explicitly run test-tools, which it seems people don't do (and it might not survivie translation to gradle, who knows), so @rmuir suggested we just move the tools into the main source tree (under src/java and src/test)

Legacy Jira details

LUCENE-8871 by Michael Sokolov (@msokolov) on Jun 19 2019, resolved Jun 29 2019

mikemccand commented 5 years ago

This has been up for a day, and is I think pretty uncontroversial - just moving files, and some code hygiene. Unless there are objections, I'll push this later today

[Legacy Jira: Michael Sokolov (@msokolov) on Jun 24 2019]

mikemccand commented 5 years ago

+1 from me. I looked thru the PR, it is just as described. Because the code moved from src/tools it suddenly requires higher standard for the build, and now tests are executed in every build. I think this is exactly what we want. Thanks Mike!

[Legacy Jira: Robert Muir (@rmuir) on Jun 25 2019]

mikemccand commented 5 years ago

Thanks for reviewing. FYI I will be delayed a bit in pushing since my primary laptop died, and I'm traveling, but will get back to this soon.

[Legacy Jira: Michael Sokolov (@msokolov) on Jun 25 2019]

mikemccand commented 5 years ago

Commit 024e200bb908c8c0b52af98c5d51e23c8faf074c in lucene-solr's branch refs/heads/master from Michael Sokolov https://gitbox.apache.org/repos/asf?p=lucene-solr.git;h=024e200

LUCENE-8871: promote kuromoji tools to main jar

[Legacy Jira: ASF subversion and git services on Jun 27 2019]

mikemccand commented 5 years ago

Pls have a look https://jenkins.thetaphi.de/job/Lucene-Solr-master-Linux/24297/

[forbidden-apis] Forbidden method invocation: java.lang.String#toUpperCase() [Uses default locale] [forbidden-apis] in org.apache.lucene.analysis.ja.util.DictionaryBuilder (DictionaryBuilder.java:45) [forbidden-apis] Scanned 128 class file(s) for forbidden API invocations (in 0.05s), 1 error(s).

[Legacy Jira: Mikhail Khludnev (@mkhludnev) on Jun 27 2019]

mikemccand commented 5 years ago

Commit 23b6a3cd3a9e0c9b96e1d92c5878748ccf07f890 in lucene-solr's branch refs/heads/master from Adrien Grand https://gitbox.apache.org/repos/asf?p=lucene-solr.git;h=23b6a3c

LUCENE-8871: Fix precommit failures.

[Legacy Jira: ASF subversion and git services on Jun 27 2019]

mikemccand commented 5 years ago

@sokolov I tried to address the precommit failures but this required some changes to the visibility of classes, could you review?

[Legacy Jira: Adrien Grand (@jpountz) on Jun 27 2019]

mikemccand commented 5 years ago

I see what you did there! Thank you for fixing. I have to say I'm really confused why this failed now, yet I am pretty sure I ran precommit earlier. I may have been distracted and forgot, but I thought I had done it. In principle the visibility changes seem OK to me, but I wonder why they were needed. I would have thought these classes were only referenced from their own package? I'm not seeing the whole picture - maybe some crosstalk between o.a.l.a.ja.dict and o.a.l.a.ja.util?

[Legacy Jira: Mike Sokolov on Jun 27 2019]

mikemccand commented 5 years ago

I see what you did there @jpountz! Thank you for fixing. I have to say I'm really confused why this failed now, yet I am pretty sure I ran precommit earlier. I may have been distracted and forgot, but I thought I had done it. In principle the visibility changes seem OK to me, but I wonder why they were needed. I would have thought these classes were only referenced from their own package? I'm not seeing the whole picture - maybe some crosstalk between o.a.l.a.ja.dict and o.a.l.a.ja.util?

[Legacy Jira: Michael Sokolov (@msokolov) on Jun 27 2019]

mikemccand commented 5 years ago

The visibility changes are required because our Javadocs checker verifies that every method/constructor that ends up in the Javadocs has a description. Reducing the visibility helped because then these classes don't even show up in javadocs anymore.

[Legacy Jira: Adrien Grand (@jpountz) on Jun 27 2019]

mikemccand commented 5 years ago

I see, thanks for explaining. I was reading the commit backwards, and thought that you had made the classes public rather than the other way around. I think all that remains here now is to back port to 8.x branch. I think that is worth doing, and safe

[Legacy Jira: Michael Sokolov (@msokolov) on Jun 27 2019]

mikemccand commented 5 years ago

Commit a9e37a6a7fd9c98b22c24b6bcd512a6da07d580c in lucene-solr's branch refs/heads/branch_8x from Michael Sokolov https://gitbox.apache.org/repos/asf?p=lucene-solr.git;h=a9e37a6

LUCENE-8871: promote kuromoji tools to main jar

[Legacy Jira: ASF subversion and git services on Jun 29 2019]

mikemccand commented 5 years ago

Closing after the 8.2.0 release

[Legacy Jira: Ignacio Vera (@iverase) on Jul 26 2019]