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

feat(#276): loading the model during compilation #303

Merged
merged 3 commits into from
Feb 11, 2025

Conversation

Marat-Tim
Copy link
Contributor

@Marat-Tim Marat-Tim commented Feb 2, 2025

In this pull request, I go through the process of getting a model file to analyze words in verbs. It used to be downloaded in runtime. I tried to fix this by downloading it to the app's resources in compile time

@Marat-Tim
Copy link
Contributor Author

I'm not sure if the downloaded file will end up in the jar being compiled, since so far I don't understand at all where and where our program builds.

@Marat-Tim
Copy link
Contributor Author

@yegor256 Please take a look

Copy link
Member

@maxonfjvipon maxonfjvipon left a comment

Choose a reason for hiding this comment

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

@Marat-Tim it would be nice to have a test that would check if the file is downloaded. Check this

@yegor256
Copy link
Member

yegor256 commented Feb 4, 2025

@Marat-Tim also, would be great if you can name your pull request better and provide a description for it, check this one please: https://www.yegor256.com/2024/12/15/open-source-beginner-advice.html

@Marat-Tim Marat-Tim changed the title feat(#276): mvp feat(#276): loading the model during compilation Feb 4, 2025
@Marat-Tim
Copy link
Contributor Author

hmm, as if testing LtTestNotVerb would test for the presence of a file
added a test, @maxonfjvipon please take a look

Assertions.assertDoesNotThrow(
() -> {
final InputStream input = new ResourceOf("en-pos-perceptron.bin").stream();
input.readAllBytes();
Copy link
Member

@maxonfjvipon maxonfjvipon Feb 5, 2025

Choose a reason for hiding this comment

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

@Marat-Tim It's not a really good idea to test file presence by reading it. The file may be huge and your OS die. There is a special mechanism and method in Java to check the file presence

@maxonfjvipon
Copy link
Member

@Marat-Tim I think I was wrong, maybe we shouldn't test things like this since your changes didn't break anything in build pipeline. Let's just remove the test, you added and I think we're good to merge

Copy link
Member

@maxonfjvipon maxonfjvipon left a comment

Choose a reason for hiding this comment

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

@yegor256 please check

@yegor256 yegor256 merged commit 462e7c8 into objectionary:master Feb 11, 2025
16 checks passed
@yegor256
Copy link
Member

@Marat-Tim thanks!

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.

3 participants