You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@lucene.apache.org by GitBox <gi...@apache.org> on 2022/05/04 21:55:55 UTC
[GitHub] [lucene] msokolov opened a new pull request, #867: LUCENE-10558: expose stream-based Kuromoji resource constructors
msokolov opened a new pull request, #867:
URL: https://github.com/apache/lucene/pull/867
Just makes some existing constructors public, and adds javadocs
--
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-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] mocobeta commented on pull request #867: LUCENE-10558: expose stream-based Kuromoji resource constructors
Posted by GitBox <gi...@apache.org>.
mocobeta commented on PR #867:
URL: https://github.com/apache/lucene/pull/867#issuecomment-1118087968
I think the same change would be needed for Nori, I don't know the use-cases but for the completeness.
--
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-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] msokolov commented on a diff in pull request #867: LUCENE-10558: expose stream-based Kuromoji resource constructors
Posted by GitBox <gi...@apache.org>.
msokolov commented on code in PR #867:
URL: https://github.com/apache/lucene/pull/867#discussion_r865801802
##########
lucene/analysis/kuromoji/src/java/org/apache/lucene/analysis/ja/dict/UnknownDictionary.java:
##########
@@ -52,18 +52,26 @@ private UnknownDictionary() throws IOException {
() -> getClassResource(DICT_FILENAME_SUFFIX));
}
- private UnknownDictionary(
- IOSupplier<InputStream> targetMapResource,
- IOSupplier<InputStream> posResource,
- IOSupplier<InputStream> dictResource)
+ /**
+ * Create a {@link UnknownDictionary} from an external resource path.
+ *
+ * @param targetMap supplier for stream containing target map
+ * @param posDict supplier for stream containing POS dictionary
+ * @param dict supplier for stream containing dictionary entries
+ * @throws IOException if a stream could not be read
+ */
+ public UnknownDictionary(
+ IOSupplier<InputStream> targetMap,
+ IOSupplier<InputStream> posDict,
+ IOSupplier<InputStream> dict)
Review Comment:
it's another day, I can no longer confirm nor deny, but Uwe's explanation makes sense to me :) If we keep this change, I'd be fine with the resource naming too, although it does have that classpath connotation?
--
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-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] uschindler commented on a diff in pull request #867: LUCENE-10558: expose stream-based Kuromoji resource constructors
Posted by GitBox <gi...@apache.org>.
uschindler commented on code in PR #867:
URL: https://github.com/apache/lucene/pull/867#discussion_r865606003
##########
lucene/analysis/kuromoji/src/java/org/apache/lucene/analysis/ja/dict/UnknownDictionary.java:
##########
@@ -52,18 +52,26 @@ private UnknownDictionary() throws IOException {
() -> getClassResource(DICT_FILENAME_SUFFIX));
}
- private UnknownDictionary(
- IOSupplier<InputStream> targetMapResource,
- IOSupplier<InputStream> posResource,
- IOSupplier<InputStream> dictResource)
+ /**
+ * Create a {@link UnknownDictionary} from an external resource path.
+ *
+ * @param targetMap supplier for stream containing target map
+ * @param posDict supplier for stream containing POS dictionary
+ * @param dict supplier for stream containing dictionary entries
+ * @throws IOException if a stream could not be read
+ */
+ public UnknownDictionary(
+ IOSupplier<InputStream> targetMap,
+ IOSupplier<InputStream> posDict,
+ IOSupplier<InputStream> dict)
Review Comment:
I think he did this because "Resource" is a bit strange, as it is no longer classpath based.
--
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-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] uschindler commented on pull request #867: LUCENE-10558: expose stream-based Kuromoji resource constructors
Posted by GitBox <gi...@apache.org>.
uschindler commented on PR #867:
URL: https://github.com/apache/lucene/pull/867#issuecomment-1118251208
I would not make the IOSupplier ctors available, they are internal only (IOSupplier is a class which is marked as subject to change).
Because we have `java.nio.files.Path` ctors for usage as replacement for the eprecated one, we need one taking `java.net.URL` for resource usage.
See #868 for an implementation.
--
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-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] mocobeta commented on a diff in pull request #867: LUCENE-10558: expose stream-based Kuromoji resource constructors
Posted by GitBox <gi...@apache.org>.
mocobeta commented on code in PR #867:
URL: https://github.com/apache/lucene/pull/867#discussion_r865500952
##########
lucene/analysis/kuromoji/src/java/org/apache/lucene/analysis/ja/dict/UnknownDictionary.java:
##########
@@ -52,18 +52,26 @@ private UnknownDictionary() throws IOException {
() -> getClassResource(DICT_FILENAME_SUFFIX));
}
- private UnknownDictionary(
- IOSupplier<InputStream> targetMapResource,
- IOSupplier<InputStream> posResource,
- IOSupplier<InputStream> dictResource)
+ /**
+ * Create a {@link UnknownDictionary} from an external resource path.
+ *
+ * @param targetMap supplier for stream containing target map
+ * @param posDict supplier for stream containing POS dictionary
+ * @param dict supplier for stream containing dictionary entries
+ * @throws IOException if a stream could not be read
+ */
+ public UnknownDictionary(
+ IOSupplier<InputStream> targetMap,
+ IOSupplier<InputStream> posDict,
+ IOSupplier<InputStream> dict)
Review Comment:
Could you explain why the parameter names should be changed? It's now inconsistent with other constructors (e.g. TokenInfoDictionary).
--
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-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] uschindler closed pull request #867: LUCENE-10558: expose stream-based Kuromoji resource constructors
Posted by GitBox <gi...@apache.org>.
uschindler closed pull request #867: LUCENE-10558: expose stream-based Kuromoji resource constructors
URL: https://github.com/apache/lucene/pull/867
--
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-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] rmuir commented on pull request #867: LUCENE-10558: expose stream-based Kuromoji resource constructors
Posted by GitBox <gi...@apache.org>.
rmuir commented on PR #867:
URL: https://github.com/apache/lucene/pull/867#issuecomment-1118443273
I don't think we shoudl do this, same reasons as stated on #868
These things should be loaded from jar as singletons and that's it.
--
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-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] uschindler commented on pull request #867: LUCENE-10558: expose stream-based Kuromoji resource constructors
Posted by GitBox <gi...@apache.org>.
uschindler commented on PR #867:
URL: https://github.com/apache/lucene/pull/867#issuecomment-1119701375
Close because superseeded by #868 and #871.
--
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-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org