uschindler commented on pull request #567: URL: https://github.com/apache/lucene/pull/567#issuecomment-1003330092
Hi @mocobeta, > > There are some remaining issues with code doing: Class.getClassLoader().getResource.... This code does not even work from inside the same module and always returns null. In most cases this is just a bug: From what I see it only affects BinaryDictionary in Kuromoji and Nori. As said before this should be refactored to not have this crazy File vs. Resource options from former times not applicable to Lucene anymore. > > There are two problems and I think we can handle them separately. > > As for the first one, "Class.getClassLoader().getResource....", all resources (dictionary data) contained in kuromoji and nori are placed in "org/apache/lucene/analysis/[ja|ko]/dict/", so they can be loaded by `org.apache.lucene.analysis.[ja|ko].dict.BinaryDictionary.class#getResourceAsStream()` by tweaking the paths a bit. A quick fix is here and it passes tests. I can make a PR or directly commit it into this branch, if you are okay with that. [mocobeta@5432b09](https://github.com/mocobeta/lucene/commit/5432b09c980f029d03c6271b3ab7f50d0a9842e7) > > As for the second one, "File vs. Resource options", resolving this is not much trivial to me. I think another issue is needed for it. I am sure we can deprecate `protected BinaryDictionary(ResourceScheme resourceScheme, String resourcePath)` and break it down into two constructors - one for resources from the class-path (no-arg constructor), another for external resources from the user's file system. The problem with your patch is that you change public API to suddenly use relative resource names in public ctors. Instead we should deprecate those and add InputStream taking ctors. I will take care of this. I wanted to do this already 10 years ago when Robert and I imported the Kuromoji code, but the number of tests affected by this was too large. By deprecting I can keep the tests unmodified at beginning and let's cleanup later. @dweiss / @mocobeta: Please make sure that you use the latest version of this PR, as I changed ALL static initializers since the one you merged. To test it outwith your branch I would solve all conflicts when merging by clicking on "theirs (whole file)" in Tortoise or similar. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org --------------------------------------------------------------------- To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org For additional commands, e-mail: issues-h...@lucene.apache.org
