chore(lib): fully refactor the library for v3

[jeertmans]

The goal of this PR is to reorganize the crate so that:

• LanguageTool API related stuff is self-contained in src/lib/. This folder should not contain anything else, except a few mandatory CLI related features, guarded behind #[cfg_attr(feature = "cli", derive(...))] and similar;
• put WordsAdd and WordsDelete inside submodules;
• put CLI related stuff inside src/cli, and renamed cli.rs to src/lib/mod.rs;
• in src/cli/check.rs, prepare the necessary logic to (1) read multiple filenames, (2) detect file type and use correct parser (i.e., Typst, Markdown, or raw text), and (3) generate a many requests as needed, splitting long text if needed.
• some work may be needed to join multiple requests that were generated using data annotation, as Typst and Markdown parser will eventually generate annotated data
• Error::ResponseEncode and Error::RequestEncode are remove and only Error::Request is used.
• A bit unrelated, but we can edit

  languagetool-rust/.github/workflows/rustlib.yml

  Line 33 in a217b40

  tag: [latest, '5.5', '5.6', '5.7', '5.8', '5.9', '6.0', '6.1']

  so that it also covers 6.2, 6.3, 6.4, and 6.5
• Add (basic) logging utilities inside CLI
• Make ltrs check README.md work (so basic splitting must be implemented)
• Add CHANGELOG.md inside the docs?
• See if we can apply changes from chore(tests): second iteration on improving coverage #91 (merging chore(tests): second iteration on improving coverage #91 will probably be too hard, as it conflicts with this PR)
• Identify Codecov report and see if we can easily improve the coverage of some parts
• Use CodSpeed for benchmarks
• Rebase changes from 3dfcc9a
• Edit docstring in src/cli/check.rs so that this or 'data' is required is changed to this, 'data', or [FILENAMES...] cannot be passed simultaneously. If nothing is specified, input will be read from stdin.

[codspeed-hq]

CodSpeed Performance Report

Merging #117 will degrade performances by 50.78%

_{Comparing v3 (880f1a9) with main (7cadfe5)}

Summary

❌ 6 (👁 6) regressions

Benchmarks breakdown

	Benchmark	BASE	HEAD	Change
👁	large	83.5 ms	169.3 ms	-50.66%
👁	medium	83.3 ms	169 ms	-50.73%
👁	small	83.1 ms	168.9 ms	-50.78%
👁	large	101 ms	186.2 ms	-45.76%
👁	medium	87.5 ms	172.9 ms	-49.41%
👁	small	83.8 ms	169.6 ms	-50.57%

[codecov]

Codecov Report

Attention: Patch coverage is 32.69514% with 457 lines in your changes missing coverage. Please review.

Project coverage is 31.55%. Comparing base (7e9e6cc) to head (880f1a9).
Report is 35 commits behind head on main.

Files with missing lines	Patch %	Lines
src/cli/check.rs	5.31%	89 Missing ⚠️
src/parsers/typst.rs	0.00%	61 Missing ⚠️
src/parsers/markdown.rs	0.00%	56 Missing ⚠️
src/parsers/html.rs	0.00%	45 Missing ⚠️
src/api/check/responses.rs	64.46%	43 Missing ⚠️
src/api/check/data_annotations.rs	36.84%	36 Missing ⚠️
src/api/server.rs	58.02%	34 Missing ⚠️
src/api/check/mod.rs	38.70%	19 Missing ⚠️
src/api/check/requests.rs	78.08%	16 Missing ⚠️
src/cli/mod.rs	14.28%	12 Missing ⚠️
... and 8 more

Additional details and impacted files

@@             Coverage Diff             @@
##             main     #117       +/-   ##
===========================================
+ Coverage   15.30%   31.55%   +16.25%     
===========================================
  Files          15       19        +4     
  Lines         732      824       +92     
===========================================
+ Hits          112      260      +148     
+ Misses        620      564       -56     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[jeertmans]

Hi @Rolv-Apneseth! So I just did some efforts to clean the CI, and also slightly updated the main comment of this PR. What would you like to work on? I don't have much time to actually implement things, as I have other work to do for my thesis ^^', but I am happy to perform reviews :-)

[Rolv-Apneseth]

Hi! I don't mind, what do you think is a reasonable next step here? Also best of luck with your thesis

[jeertmans]

Hi! I don't mind, what do you think is a reasonable next step here? Also best of luck with your thesis

Thanks! Can you just check the to-do list above, and see what you can do?

[Rolv-Apneseth]

Sure, so maybe for the first one I can just move all API code to a src/lib/ directory, and move cli stuff to src/cli/mod.rs? WordsAdd and WordsDelete should already be in submodules.

Also would you mind explaining the reasoning behind the cli feature? I thought that would just be separated out entirely and only used in main.rs

[jeertmans]

Sure, so maybe for the first one I can just move all API code to a src/lib/ directory, and move cli stuff to src/cli/mod.rs? WordsAdd and WordsDelete should already be in submodules.

Sure, looks good!

Also would you mind explaining the reasoning behind the cli feature? I thought that would just be separated out entirely and only used in main.rs

Of course :-)
Two main reasons:

1. People may want to use LTRS as a crate, and I don't want to enforce them to download and compile the code that is only used for building the CLI. Using a feature allows excluding this part of the library.
2. I don't want to pollute the "language tool API binding" parts with too much code that is not related to it. The only exceptions are the derive macros that are way more convenient to put directly on the original request structures.

Did I answer your question?

[Rolv-Apneseth]

Not quite sorry. I understand being able to disable it, but what is the benefit of enabling it? Why not just have the CLI stuff separated entirely from the library side, and only used in the binary

[jeertmans]

Not quite sorry. I understand being able to disable it, but what is the benefit of enabling it? Why not just have the CLI stuff separated entirely from the library side, and only used in the binary

There is too much things to be put inside just main.rs

Creating a sub crate allows to have a cleaner workspace, and testing libraries is also easier. Finally, people could be interested to have the CLI tools exposed in the public API, e.g., to create a wrapper application. There may be better project organization, and I am of course open to suggestion.

[jeertmans]

I think it's ready to be merged, what do you think @Rolv-Apneseth?

[Rolv-Apneseth]

Just a couple things I think:

1. What about those 2 failing checks?
2. Should we run the tool against the CONTRIBUTING.md to check for errors - e.g. I noticed To avoid spamming the free LanguageTool API, running the tests required <-(requires) you to specify the LanguageTool server URL and PORT
3. I think cargo doc, cargo msrv verify -- cargo check --all-features and cargo nextest run --all-features should be mentioned in that contributing guide too. And accepting new snapshots with cargo insta review.
4. Is that action which comments the (incorrect) spelling errors on the PR fixed? I don't see it any more so it may be, but I did try to mark it as spam since it was giving me too many notifications

[jeertmans]

Just a couple things I think:

1. What about those 2 failing checks?

Codacy errors are not really meaningful, because it is just complaining about lines being too long in Markdown files, but I don't know how to ignore those globally.
CodSpeed errors aren't really meaningful either, because it complains about a possible regression (but the library has changed so much that a 50% regression isn't really a problem, nor representative).

2. Should we run the tool against the CONTRIBUTING.md to check for errors - e.g. I noticed To avoid spamming the free LanguageTool API, running the tests required <-(requires) you to specify the LanguageTool server URL and PORT

Yep, I wrote that quickly and have probably left some typos :'-)

3. I think cargo doc, cargo msrv verify -- cargo check --all-features and cargo nextest run --all-features should be mentioned in that contributing guide too. And accepting new snapshots with cargo insta review.

Good point! I'll fix this, but can you add the necessary information for insta snapshots?

4. Is that action which comments the (incorrect) spelling errors on the PR fixed? I don't see it any more so it may be, but I did try to mark it as spam since it was giving me too many notifications

Yes, most messages can be ignored (it's a bit painful to check all messages, but I did try my best).

[Rolv-Apneseth]

Codacy errors are not really meaningful, because it is just complaining about lines being too long in Markdown files, but I don't know how to ignore those globally.

What actually is codacy? Is there not configuration for the line length? And I see there's errors coming from the typst file for some reason.

CodSpeed errors aren't really meaningful either, because it complains about a possible regression (but the library has changed so much that a 50% regression isn't really a problem, nor representative).

I think it says on the action that you need to acknowledge the performance decrease.

Yep, I wrote that quickly and have probably left some typos :'-)

So should I add it to where we run the readme through ltrs in our test suite?

Good point! I'll fix this, but can you add the necessary information for insta snapshots?

Sure yeah, but all there is to say is that running tests might create changes to the snapshots, and you'll need to accept changes (assuming they are intentional) with cargo insta review.

Yes, most messages can be ignored (it's a bit painful to check all messages, but I did try my best).

Would we turn off whatever action is causing that or you want to keep it?

[jeertmans]

What actually is codacy? Is there not configuration for the line length? And I see there's errors coming from the typst file for some reason.

It's a code-quality checker, but it runs online. I am disabling line-length checks locally (see the config files), but for some reason it is not taken into account by Codacy, and I couldn't find documentation about this online.

I think it says on the action that you need to acknowledge the performance decrease.

Yes, you're correct! I just acknowledged them.

So should I add it to where we run the readme through ltrs in our test suite?

Not sure to understand this, could you clarify?

Sure yeah, but all there is to say is that running tests might create changes to the snapshots, and you'll need to accept changes (assuming they are intentional) with cargo insta review.

Makes sense, let's just add this at the end of the Testing section in CONTRIBUTING.md.

Would we turn off whatever action is causing that or you want to keep it?

I think it would make sense to turn this off for at least some code files. I'll try first to disable a few rules, similar to what I did for another repository I own. If that spamming again the PRs, I'll disable checking code files.

[Rolv-Apneseth]

Not sure to understand this, could you clarify?

Sure. In our own CLI tests, we snapshot test the README.md file, should I add a snapshot for CONTRIBUTING.md too?

Makes sense, let's just add this at the end of the Testing section in CONTRIBUTING.md.

Alright

If that spamming again the PRs, I'll disable checking code files.

Sounds good

[jeertmans]

Not sure to understand this, could you clarify?

Sure. In our own CLI tests, we snapshot test the README.md file, should I add a snapshot for CONTRIBUTING.md too?

Yes, good idea!

[Rolv-Apneseth]

If that action is just checking the readme, do we still need it? It's pointing out that "GitHub" misspelling in links...

[jeertmans]

If that action is just checking the readme, do we still need it? It's pointing out that "GitHub" misspelling in links...

At the moment it is also checking the text found in Rust files, so likely checking the documentation too, but I disabled a few rules to avoid false positives.

[Rolv-Apneseth]

Alright fair enough. Overall, LGTM anyway

[jeertmans]

Nice, thanks for your help @Rolv-Apneseth! I've merged this and will soon release an alpha version. Hopefully, it will work just fine, and we can release the v3 in one or two weeks.

[Rolv-Apneseth]

Fantastic