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-10558: expose stream-based Kuromoji resource constructors #867

Closed
wants to merge 1 commit into from

Conversation

msokolov
Copy link
Contributor

@msokolov msokolov commented May 4, 2022

Just makes some existing constructors public, and adds javadocs

Comment on lines +64 to +66
IOSupplier<InputStream> targetMap,
IOSupplier<InputStream> posDict,
IOSupplier<InputStream> dict)
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you explain why the parameter names should be changed? It's now inconsistent with other constructors (e.g. TokenInfoDictionary).

Copy link
Contributor

Choose a reason for hiding this comment

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

I think he did this because "Resource" is a bit strange, as it is no longer classpath based.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

@mocobeta
Copy link
Contributor

mocobeta commented May 5, 2022

I think the same change would be needed for Nori, I don't know the use-cases but for the completeness.

@uschindler
Copy link
Contributor

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.

Copy link
Contributor

@uschindler uschindler left a comment

Choose a reason for hiding this comment

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

I'd go with #868

@rmuir
Copy link
Member

rmuir commented May 5, 2022

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.

@uschindler
Copy link
Contributor

Close because superseeded by #868 and #871.

@uschindler uschindler closed this May 6, 2022
@msokolov msokolov deleted the LUCENE-10558 branch August 21, 2022 14:14
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.

None yet

4 participants